From c75b715b3b34065f115140865e73815e57889ed5 Mon Sep 17 00:00:00 2001 From: cjkas Date: Tue, 13 Oct 2026 21:17:02 +0200 Subject: [PATCH] refactor: share step targets between remote and group steps A received step and a group step each worked out where a step takes the shade, the group copy with its own inline maths. stepTargets() in ShadeMotion now does it for both: a tilt-only shade steps its tilt, an integrated tilt turns its slats to the end first and then steps the lift, and every other shade steps its lift. Only edge cases change. A group step on a tilt-only shade already at the end now goes through like a remote step, and a step that can't be sized (no travel time) leaves the targets alone in both. Co-Authored-By: Claude Opus 5.5 --- src/ShadeMotion.cpp | 41 +++++++++ src/ShadeMotion.h | 20 +++++ src/Somfy.h | 2 + src/SomfyShade.cpp | 147 +++---------------------------- test/test_motion/test_motion.cpp | 84 ++++++++++++++++++ 5 files changed, 160 insertions(+), 134 deletions(-) diff --git a/src/ShadeMotion.cpp b/src/ShadeMotion.cpp index af30197..54a8f75 100644 --- a/src/ShadeMotion.cpp +++ b/src/ShadeMotion.cpp @@ -36,6 +36,47 @@ bool stepPosition(float pos, int8_t dir, uint32_t travelMs, uint16_t stepSize, u out = dir > 0 ? std::min(100.0f, pos + step) : std::max(0.0f, pos - step); return true; } +StepTargets stepTargets(tilt_types tilt, int8_t dir, float pos, float tiltPos, + uint32_t upTime, uint32_t downTime, uint32_t tiltTime, uint16_t stepSize, uint8_t frameStepSize) { + StepTargets t; + if(stepSize == 0) return t; + auto atEnd = [dir](float p) { return dir > 0 ? p >= 100.0f : p <= 0.0f; }; + const uint32_t liftTime = dir > 0 ? downTime : upTime; + float step; + if(tilt == tilt_types::integrated) { + if(atEnd(tiltPos) && atEnd(pos)) return t; + if(!atEnd(tiltPos)) { + if(!stepPosition(tiltPos, dir, tiltTime, stepSize, frameStepSize, step)) return t; + t.setLift = true; + t.lift = pos; + t.setTilt = true; + t.tilt = step; + } + else { + if(!stepPosition(pos, dir, liftTime, stepSize, frameStepSize, step)) return t; + t.setLift = true; + t.lift = step; + t.setTilt = true; + t.tilt = tiltPos; + } + } + else if(tilt == tilt_types::tiltonly) { + if(!stepPosition(tiltPos, dir, tiltTime, stepSize, frameStepSize, step)) return t; + t.setTilt = true; + t.tilt = step; + } + else { + if(atEnd(pos)) { + t.action = StepAction::none; + return t; + } + if(!stepPosition(pos, dir, liftTime, stepSize, frameStepSize, step)) return t; + t.setLift = true; + t.lift = step; + } + t.action = StepAction::move; + return t; +} void TxHold::mark(uint32_t ticket, bool moving, uint32_t now) { if(ticket == 0 || this->ticket != 0 || moving) return; this->ticket = ticket; diff --git a/src/ShadeMotion.h b/src/ShadeMotion.h index ab27b9a..1fa10bd 100644 --- a/src/ShadeMotion.h +++ b/src/ShadeMotion.h @@ -2,6 +2,7 @@ #define SHADE_MOTION_H #include #include "TxBuffer.h" +#include "ShadeTypes.h" // Shade position maths. Kept free of the shade state, radio and network so it // can be unit tested on the PC (pio test -e native). @@ -20,6 +21,25 @@ float limitTarget(int8_t dir); // ms of the travelMs a full travel takes in that direction. False when // travelMs or stepSize is 0, so the step is ignored. bool stepPosition(float pos, int8_t dir, uint32_t travelMs, uint16_t stepSize, uint8_t frameStepSize, float &out); +// What a step command does to a shade. +enum class StepAction : uint8_t { + ignore, // The step can't be sized or the shade is at both ends: drop it. + none, // A lift shade already at the end: nothing moves. + move // Set the targets below. +}; +struct StepTargets { + StepAction action = StepAction::ignore; + bool setLift = false; + float lift = 0.0f; + bool setTilt = false; + float tilt = 0.0f; +}; +// Where a step in dir (> 0 down, < 0 up) takes a shade at pos and tiltPos. A +// tilt-only shade steps its tilt. An integrated tilt turns its slats to the +// end first, holding the lift, and then steps the lift. Every other shade +// steps its lift. +StepTargets stepTargets(tilt_types tilt, int8_t dir, float pos, float tiltPos, + uint32_t upTime, uint32_t downTime, uint32_t tiltTime, uint16_t stepSize, uint8_t frameStepSize); // What a held shade does on this pass of checkMovement(). enum class TxHoldCheck { diff --git a/src/Somfy.h b/src/Somfy.h index a61c97a..ba513cc 100644 --- a/src/Somfy.h +++ b/src/Somfy.h @@ -136,6 +136,8 @@ class SomfyShade : public SomfyRemote { void runToLimit(int8_t dir, bool tilt, uint32_t since); // Sets the targets the sun or wind sends the shade to. void applySensorMove(SensorMove move); + // Sets the targets for a step in dir. False when the step is dropped. + bool applyStep(int8_t dir, uint8_t frameStepSize); public: uint8_t roomId = 0; int8_t sortOrder = 0; diff --git a/src/SomfyShade.cpp b/src/SomfyShade.cpp index 206215b..3e1607b 100644 --- a/src/SomfyShade.cpp +++ b/src/SomfyShade.cpp @@ -529,7 +529,6 @@ void SomfyShade::processFrame(somfy_frame_t &frame, bool internal) { const uint64_t curTime = millis(); this->lastFrame.copy(frame); int8_t dir = 0; - float step; // Where a step command takes the shade. this->moveStart = this->tiltStart = curTime; this->startPos = this->currentPos; this->startTiltPos = this->currentTiltPos; @@ -715,40 +714,8 @@ void SomfyShade::processFrame(somfy_frame_t &frame, bool internal) { this->lastFrame.processed = true; if(this->shadeType == shade_types::drycontact || this->shadeType == shade_types::drycontact2) return; dir = 0; - // With the step commands and integrated shades - // the motor must tilt in the direction first then move - // so we have to calculate the target with this in mind. - if(this->stepSize == 0) return; // Avoid divide by 0. if(this->lastFrame.stepSize == 0) this->lastFrame.stepSize = 1; - if(this->tiltType == tilt_types::integrated) { - // With integrated tilt this is more involved than ne would think because the step command can be moving not just the tilt - // but the lift. So a determination needs to be made as to whether we are currently moving and it should stop. - // Conditions: - // 1. If both the tilt and lift are at 0% do nothing - // 2. If the tilt position is not currently at the top then shift the tilt. - // 3. If the tilt position is not currently at the top then shift the lift. - if(this->currentTiltPos <= 0.0f && this->currentPos <= 0.0f) return; // Do nothing - else if(this->currentTiltPos > 0.0f) { - // Set the tilt position. This should stop the lift movement. - this->p_target(this->currentPos); - if(!stepPosition(this->currentTiltPos, -1, this->tiltTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_tiltTarget(step); - } - else { - // We only have the lift to move. - if(!stepPosition(this->currentPos, -1, this->upTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_tiltTarget(this->currentTiltPos); - this->p_target(step); - } - } - else if(this->tiltType == tilt_types::tiltonly) { - if(!stepPosition(this->currentTiltPos, -1, this->tiltTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_tiltTarget(step); - } - else if(this->currentPos > 0.0f) { - if(!stepPosition(this->currentPos, -1, this->upTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_target(step); - } + if(!this->applyStep(-1, this->lastFrame.stepSize)) return; this->emitCommand(cmd, internal ? "internal" : "remote", frame.remoteAddress); break; case somfy_commands::StepDown: @@ -756,41 +723,8 @@ void SomfyShade::processFrame(somfy_frame_t &frame, bool internal) { this->lastFrame.processed = true; if(this->shadeType == shade_types::drycontact || this->shadeType == shade_types::drycontact2) return; dir = 1; - // With the step commands and integrated shades - // the motor must tilt in the direction first then move - // so we have to calculate the target with this in mind. - if(this->stepSize == 0) return; // Avoid divide by 0. if(this->lastFrame.stepSize == 0) this->lastFrame.stepSize = 1; - - if(this->tiltType == tilt_types::integrated) { - // With integrated tilt this is more involved than ne would think because the step command can be moving not just the tilt - // but the lift. So a determination needs to be made as to whether we are currently moving and it should stop. - // Conditions: - // 1. If both the tilt and lift are at 100% do nothing - // 2. If the tilt position is not currently at the bottom then shift the tilt. - // 3. If the tilt position is add the bottom then shift the lift. - if(this->currentTiltPos >= 100.0f && this->currentPos >= 100.0f) return; // Do nothing - else if(this->currentTiltPos < 100.0f) { - // Set the tilt position. This should stop the lift movement. - this->p_target(this->currentPos); - if(!stepPosition(this->currentTiltPos, 1, this->tiltTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_tiltTarget(step); - } - else { - // We only have the lift to move. - this->p_tiltTarget(this->currentTiltPos); - if(!stepPosition(this->currentPos, 1, this->downTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_target(step); - } - } - else if(this->tiltType == tilt_types::tiltonly) { - if(!stepPosition(this->currentTiltPos, 1, this->tiltTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_tiltTarget(step); - } - else if(this->currentPos < 100.0f) { - if(!stepPosition(this->currentPos, 1, this->downTime, this->stepSize, this->lastFrame.stepSize, step)) return; - this->p_target(step); - } + if(!this->applyStep(1, this->lastFrame.stepSize)) return; this->emitCommand(cmd, internal ? "internal" : "remote", frame.remoteAddress); break; case somfy_commands::Toggle: @@ -896,75 +830,12 @@ void SomfyShade::processInternalCommand(somfy_commands cmd, uint8_t repeat) { } break; case somfy_commands::StepUp: - // With the step commands and integrated shades - // the motor must tilt in the direction first then move - // so we have to calculate the target with this in mind. - if(this->stepSize == 0) return; // Avoid divide by 0. - if(this->tiltType == tilt_types::integrated) { - // With integrated tilt this is more involved than ne would think because the step command can be moving not just the tilt - // but the lift. So a determination needs to be made as to whether we are currently moving and it should stop. - // Conditions: - // 1. If both the tilt and lift are at 0% do nothing - // 2. If the tilt position is not currently at the top then shift the tilt. - // 3. If the tilt position is not currently at the top then shift the lift. - if(this->currentTiltPos <= 0.0f && this->currentPos <= 0.0f) return; // Do nothing - else if(this->currentTiltPos > 0.0f) { - // Set the tilt position. This should stop the lift movement. - this->p_target(this->currentPos); - if(this->tiltTime == 0) return; // Avoid divide by 0. - this->p_tiltTarget(max(0.0f, this->currentTiltPos - (100.0f/(static_cast(this->tiltTime/static_cast(this->stepSize)))))); - } - else { - // We only have the lift to move. - if(this->upTime == 0) return; // Avoid divide by 0. - this->p_tiltTarget(this->currentTiltPos); - this->p_target(max(0.0f, this->currentPos - (100.0f/(static_cast(this->upTime/static_cast(this->stepSize)))))); - } - } - else if(this->tiltType == tilt_types::tiltonly) { - if(this->tiltTime == 0 || this->currentTiltPos <= 0.0f) return; - this->p_tiltTarget(max(0.0f, this->currentTiltPos - (100.0f/(static_cast(this->tiltTime/static_cast(this->stepSize)))))); - } - else if(this->currentPos > 0.0f) { - if(this->upTime == 0) return; - this->p_target(max(0.0f, this->currentPos - (100.0f/(static_cast(this->upTime/static_cast(this->stepSize)))))); - } + // A group step is always one step. + if(!this->applyStep(-1, 1)) return; break; case somfy_commands::StepDown: dir = 1; - // With the step commands and integrated shades - // the motor must tilt in the direction first then move - // so we have to calculate the target with this in mind. - if(this->stepSize == 0) return; // Avoid divide by 0. - if(this->tiltType == tilt_types::integrated) { - // With integrated tilt this is more involved than ne would think because the step command can be moving not just the tilt - // but the lift. So a determination needs to be made as to whether we are currently moving and it should stop. - // Conditions: - // 1. If both the tilt and lift are at 100% do nothing - // 2. If the tilt position is not currently at the bottom then shift the tilt. - // 3. If the tilt position is add the bottom then shift the lift. - if(this->currentTiltPos >= 100.0f && this->currentPos >= 100.0f) return; // Do nothing - else if(this->currentTiltPos < 100.0f) { - // Set the tilt position. This should stop the lift movement. - this->p_target(this->currentPos); - if(this->tiltTime == 0) return; // Avoid divide by 0. - this->p_tiltTarget(min(100.0f, this->currentTiltPos + (100.0f/(static_cast(this->tiltTime/static_cast(this->stepSize)))))); - } - else { - // We only have the lift to move. - if(this->downTime == 0) return; // Avoid divide by 0. - this->p_tiltTarget(this->currentTiltPos); - this->p_target(min(100.0f, this->currentPos + (100.0f/(static_cast(this->downTime/static_cast(this->stepSize)))))); - } - } - else if(this->tiltType == tilt_types::tiltonly) { - if(this->tiltTime == 0 || this->stepSize == 0 || this->currentTiltPos >= 100.0f) return; - this->p_tiltTarget(min(100.0f, this->currentTiltPos + (100.0f/(static_cast(this->tiltTime/static_cast(this->stepSize)))))); - } - else if(this->currentPos < 100.0f) { - if(this->downTime == 0 || this->stepSize == 0) return; - this->p_target(min(100.0f, this->currentPos + (100.0f/(static_cast(this->downTime/static_cast(this->stepSize)))))); - } + if(!this->applyStep(1, 1)) return; break; case somfy_commands::Flag: this->p_sunFlag(false); @@ -1097,6 +968,14 @@ void SomfyShade::applySensorMove(SensorMove move) { if(t.setLift) this->p_target(t.lift); if(t.setTilt) this->p_tiltTarget(t.tilt); } +bool SomfyShade::applyStep(int8_t dir, uint8_t frameStepSize) { + const StepTargets t = stepTargets(this->tiltType, dir, this->currentPos, this->currentTiltPos, + this->upTime, this->downTime, this->tiltTime, this->stepSize, frameStepSize); + if(t.action == StepAction::ignore) return false; + if(t.setLift) this->p_target(t.lift); + if(t.setTilt) this->p_tiltTarget(t.tilt); + return true; +} void SomfyShade::setMovement(int8_t dir) { int8_t currDir = this->direction; int8_t currTiltDir = this->tiltDirection; diff --git a/test/test_motion/test_motion.cpp b/test/test_motion/test_motion.cpp index e94afa2..6996a42 100644 --- a/test/test_motion/test_motion.cpp +++ b/test/test_motion/test_motion.cpp @@ -210,6 +210,81 @@ static void test_txhold_times_out_across_millis_wrap() { TEST_ASSERT_TRUE(hold.timedOut(0x00007000u, 30000)); // 32768ms later } +/********************************************************************* + * stepTargets + ********************************************************************/ +// One step of 100ms: 0.5% of the 20s up travel, 1% of the 10s down travel and +// 2% of the 5s tilt travel. +static StepTargets stepOf(tilt_types tilt, int8_t dir, float pos, float tiltPos, uint8_t frameSteps = 1, + uint32_t upTime = 20000, uint32_t downTime = 10000, uint32_t tiltTime = 5000, uint16_t stepSize = 100) { + return stepTargets(tilt, dir, pos, tiltPos, upTime, downTime, tiltTime, stepSize, frameSteps); +} +#define ASSERT_ACTION(expected, actual) TEST_ASSERT_EQUAL_UINT8(static_cast(expected), static_cast(actual)) + +static void test_stepTargets_lift_shade_steps_its_lift() { + StepTargets t = stepOf(tilt_types::none, DOWN, 40, 0); + ASSERT_ACTION(StepAction::move, t.action); + TEST_ASSERT_TRUE(t.setLift); + ASSERT_POS(41, t.lift); + TEST_ASSERT_FALSE(t.setTilt); + // Up uses the up travel time. + t = stepOf(tilt_types::none, UP, 40, 0); + ASSERT_POS(39.5f, t.lift); +} +static void test_stepTargets_frame_steps_multiply_the_step() { + ASSERT_POS(43, stepOf(tilt_types::none, DOWN, 40, 0, 3).lift); +} +static void test_stepTargets_tilt_motors_step_the_lift() { + // A separate tilt motor or euromode shade tilts with a long press, not a step. + ASSERT_POS(41, stepOf(tilt_types::tiltmotor, DOWN, 40, 50).lift); + TEST_ASSERT_FALSE(stepOf(tilt_types::euromode, DOWN, 40, 50).setTilt); +} +static void test_stepTargets_lift_shade_at_the_end_has_nothing_to_do() { + ASSERT_ACTION(StepAction::none, stepOf(tilt_types::none, DOWN, 100, 0).action); + ASSERT_ACTION(StepAction::none, stepOf(tilt_types::none, UP, 0, 0).action); +} +static void test_stepTargets_tilt_only_shade_steps_its_tilt() { + StepTargets t = stepOf(tilt_types::tiltonly, DOWN, 100, 40); + ASSERT_ACTION(StepAction::move, t.action); + TEST_ASSERT_FALSE(t.setLift); + TEST_ASSERT_TRUE(t.setTilt); + ASSERT_POS(42, t.tilt); + // At the end the tilt stays there. + t = stepOf(tilt_types::tiltonly, UP, 100, 0); + ASSERT_ACTION(StepAction::move, t.action); + ASSERT_POS(0, t.tilt); +} +static void test_stepTargets_integrated_tilts_first() { + StepTargets t = stepOf(tilt_types::integrated, DOWN, 20, 40); + ASSERT_ACTION(StepAction::move, t.action); + // The lift holds where it is while the slats turn. + TEST_ASSERT_TRUE(t.setLift); + ASSERT_POS(20, t.lift); + TEST_ASSERT_TRUE(t.setTilt); + ASSERT_POS(42, t.tilt); +} +static void test_stepTargets_integrated_lifts_once_tilted() { + StepTargets t = stepOf(tilt_types::integrated, DOWN, 20, 100); + ASSERT_ACTION(StepAction::move, t.action); + ASSERT_POS(21, t.lift); + ASSERT_POS(100, t.tilt); + t = stepOf(tilt_types::integrated, UP, 50, 0); + ASSERT_POS(49.5f, t.lift); + ASSERT_POS(0, t.tilt); +} +static void test_stepTargets_integrated_at_both_ends_is_ignored() { + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::integrated, DOWN, 100, 100).action); + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::integrated, UP, 0, 0).action); +} +static void test_stepTargets_without_a_travel_time_or_step_size_is_ignored() { + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::none, DOWN, 40, 0, 1, 20000, 0).action); + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::none, UP, 40, 0, 1, 0).action); + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::tiltonly, DOWN, 100, 40, 1, 20000, 10000, 0).action); + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::integrated, DOWN, 20, 40, 1, 20000, 10000, 0).action); + // Even a shade with nothing to do ignores a step it can't size. + ASSERT_ACTION(StepAction::ignore, stepOf(tilt_types::none, DOWN, 100, 0, 1, 20000, 10000, 5000, 0).action); +} + int main(int argc, char **argv) { (void)argc; (void)argv; @@ -227,6 +302,15 @@ int main(int argc, char **argv) { RUN_TEST(test_step_stops_at_the_ends); RUN_TEST(test_step_is_ignored_without_a_travel_time_or_step_size); RUN_TEST(test_step_of_no_frame_steps_stays_put); + RUN_TEST(test_stepTargets_lift_shade_steps_its_lift); + RUN_TEST(test_stepTargets_frame_steps_multiply_the_step); + RUN_TEST(test_stepTargets_tilt_motors_step_the_lift); + RUN_TEST(test_stepTargets_lift_shade_at_the_end_has_nothing_to_do); + RUN_TEST(test_stepTargets_tilt_only_shade_steps_its_tilt); + RUN_TEST(test_stepTargets_integrated_tilts_first); + RUN_TEST(test_stepTargets_integrated_lifts_once_tilted); + RUN_TEST(test_stepTargets_integrated_at_both_ends_is_ignored); + RUN_TEST(test_stepTargets_without_a_travel_time_or_step_size_is_ignored); RUN_TEST(test_displayPosition); RUN_TEST(test_flipCommand_swaps_directions); RUN_TEST(test_flipCommand_keeps_the_rest);