arduino-shutters icon indicating copy to clipboard operation
arduino-shutters copied to clipboard

_currentLevel == _targetLevel not handled properly

Open alexelite opened this issue 3 years ago • 0 comments

Firstly, thank you @marvinroger for this library.

I did some testing for my custom usage and found 2 situations when state machine wrongly changes from idle to targeting.

Initial condition: All testing is done using the latest github version and with the default example, using Arduino IDE, and an Arduino UNO type board. The only changes I made was to add 'u' after first number for upCourseTime and downCourseTime definition and setLevel(30) changed to setLevel(0) in setup.

const unsigned long upCourseTime = 30u * 1000;
const unsigned long downCourseTime = 45u * 1000;

I found 2 bad behaviors as folows Bad behavior 1: During a reset, if setLevel(0) is called, when reset finishes, state machine switches from RESETTING to IDLE to TARGETING and then into a RESETTING loop because _currentLevel goes outside normal 0-100 range (to 255). Calling for any other level fixes the loop. Evaluating _targetLevel == _currentLevel and asigning LEVEL_NONE to _targetLevel if true, keeps the state machine in IDLE.

adding this line in loop function:
if (_targetLevel == _currentLevel) _targetLevel = LEVEL_NONE;
before:
if (_state == STATE_IDLE && _targetLevel == LEVEL_NONE) return *this; // nothing to do
solves the problem

Bad behavior 2: If during an 100% end run calibration, setLevel(100) is called, after calibration finishes, the state machine goes into TARGETING and goes to 0% and then returns to 100%. In setLevel function, calibration state is not evaluated and _targetLevel is changed from LEVEL_NONE to 100. State machine switches from IDLE to TARGETING because of this. At this point direction is set to UP and machine moves to 0, IDLES, but still having a target set, starts moving down to 100. Calling a setLevel during this move fixes the behavior. Same line as before fixes the problem, but can also be fixed before by changing:

in setLevel function this:
if (_state == STATE_IDLE && level == _currentLevel) return *this;
to:
if (_state == STATE_IDLE || _state == STATE_CALIBRATING) && level == _currentLevel) return *this;

I currently use only the first change, and both problems are gone.

alexelite avatar Jan 19 '21 13:01 alexelite