Rotary encoder only works in one direction (and two other bugs)

[MyriadBugs]

PROBLEM DESCRIPTION

EDITED:
I missed part of the code for rotary interrupt support. This part of the issue was user error. The other bugs were real and were fixed.

The commit 541443f to support_rotary.ino broke rotary support for anyone except those specifying the "Xiaomi Mi Desk" rotary configuration (i.e. not using the OPTION_A 5 setting).

By selecting ROTARY_OPTION3 as the "default" the ISR (RotaryIsArg) is only called on the falling edge of channel A. Going back to the first implementation of rotary support I'm not sure how this option ever worked as it will only capture incremental encoders in the same state (Channel B will always be high on the rising edge of A and will always be low on the falling edge of A).

                              _______         _______
            GPIO_ROT1A ______|       |_______|       |______ GPIO_ROT1A
negative <--              _______         _______         __            --> positive
            GPIO_ROT1B __|       |_______|       |_______|   GPIO_ROT1B

It could be fixed by either making the code match the MiDesk ISR (i.e. trigger on change for both Channel A and B, then check both pins in the ISR), or, if you really want to have one fewer interrupts at the cost of halving the encoder resolution you could change the interrupt to trigger on CHANGE instead of RISING or FALLING. I'd probably lean towards just merging the ISRs and removing the debounce code but another solution would be to change the interrupt trigger at support_rotary.ino#L179 to CHANGE.

I found at least one reference to someone who noticed their encoder only works in one direction (https://discord.com/channels/479389167382691863/1096811519414632619/1112403559783350352)

OTHER BUGS:

1. The LightColorOffset function in xdrv_04_light.ino creates an unsigned int underflow instead of wrapping from 0 to 359. The effect is that if you try to offset down through the color wheel you'll get wrapped back around to 65535 and it won't work again until you spin yourself all the way back down to 359. For reference the fix I did is below. Changed first 4 lines of LightColorOffset@xdrv_04_light.ino#L1357

void LightColorOffset(int32_t offset) {
  int16_t hue;
  uint8_t sat;
  light_state.getHSB(reinterpret_cast<uint16_t*>(&hue), &sat, nullptr); 
  offset = max(min(offset, 359),-359);

2. Increasing SetOption43 above 100 (or above 33 if using OPTION_A 5) will break the dimming function (uint division of 100 by >100 is 0). Increasing above 350/3 or 360/3 will then break CT and Color controls respectively. The quick fix I used is below for reference but I'll eventually rewrite my own fork to support fractional steps and I'd be inclined to say that that's the real fix 🤷‍♀️

  Rotary.dimmer_increment = 100 / min((uint8_t)100, max_steps);  // Dimmer 1..100 = 100
  Rotary.ct_increment =     350 / min((uint8_t)350, max_steps);  // Ct 153..500 = 347
  Rotary.color_increment =  360 / min((uint8_t)360, max_steps);  // Hue 0..359 = 360

REQUESTED INFORMATION

Make sure your have performed every step and checked the applicable boxes before submitting your issue. Thank you!

• Read the Contributing Guide and Policy and the Code of Conduct
• Searched the problem in issues
• Searched the problem in discussions
• Searched the problem in the docs
• Searched the problem in the chat
• Device used (e.g., Sonoff Basic): Custom, can be reproduced with an esp8266 and a rotary encoder.
• Tasmota binary firmware version number used: 13.0.0.2(tasmota) (2_7_4_9/2.2.2-dev(38a443e))
  • Pre-compiled
  • Self-compiled
• Flashing tools used: PlatformIO and USB
• Provide the output of command: Backlog Template; Module; GPIO 255:

  Configuration output here:
NOTE: This is with OPTION_A 5 enabled and is working, without OPTION_A 5 the rotary is broken as described.
03:59:07.886 CMD: Grp 0, Cmd 'BACKLOG', Idx 1, Len 26, Pld -99, Data 'Template; Module; GPIO 255'
03:59:07.962 SRC: Backlog
03:59:07.965 CMD: Grp 0, Cmd 'TEMPLATE', Idx 1, Len 0, Pld -99, Data ''
03:59:07.971 MQT: stat/test-light/RESULT = {"NAME":"TestLight","GPIO":[32,5732,33,0,3296,3264,0,0,418,417,416,419,0,0],"FLAG":0,"BASE":18}
03:59:08.190 SRC: Backlog
03:59:08.193 CMD: Grp 0, Cmd 'MODULE', Idx 1, Len 0, Pld -99, Data ''
03:59:08.198 MQT: stat/test-light/RESULT = {"Module":{"0":"TestLight"}}
03:59:08.440 SRC: Backlog
03:59:08.443 CMD: Grp 0, Cmd 'GPIO', Idx 1, Len 3, Pld 255, Data '255'
03:59:08.454 MQT: stat/test-light/RESULT = {"GPIO0":{"32":"Button1"},"GPIO1":{"5732":"Option A5"},"GPIO2":{"33":"Button2"},"GPIO3":{"0":"None"},"GPIO4":{"3296":"Rotary B1"},"GPIO5":{"3264":"Rotary A1"},"GPIO9":{"0":"None"},"GPIO10":{"0":"None"},"GPIO12":{"418":"PWM3"},"GPIO13":{"417":"PWM2"},"GPIO14":{"416":"PWM1"},"GPIO15":{"419":"PWM4"},"GPIO16":{"0":"None"},"GPIO17":{"0":"None"}}

• If using rules, provide the output of this command: Backlog Rule1; Rule2; Rule3:

  Rules output here:

• Provide the output of this command: Status 0:

  STATUS 0 output here:

• Set weblog to 4 and then, when you experience your issue, provide the output of the Console log:

  Console output here:

TO REPRODUCE

1. Setup an ESP8266 with latest tasmota
4. Connect an incremental rotary encoder
5. Configure two rotary encoder pins as "Rotary A 1" and Rotary B 1"
6. Enable something to debug (add rules, connect a light, etc)
7. Turn rotary encoder
8. Rotary encoder will only increment (or decrement if you swap the A and B pins).

Additional Bugs:

SetOptions43 101: Rotate the encoder in either direction and the dimming will not change.

SetOption98 0 (default): Holding Button 1 (index 0) and rotating encoder "down" will cause an underflow and set the hue to 65535. Hue will be stuck > 359 until you've spun the encoder enough to trigger the call to LightColorOffset ≈ 100 times.

EXPECTED BEHAVIOUR

Option_A 5 not set: Rotary encoder should increment in one direction and decrement in another.

SetOption98 0 (default): holding button 1 (index 0) and rotating the encoder left should reduce the hue and eventually wrap around back to 359.

SetOption98 0 (default): Setting the SetOption43 "too high" shouldn't silently break the dimming/color control functions, worst case scenario they should just be capped at the maximum supported resolution. Best case scenario the encoder support would allow fractional step sizes (storing the fractions and calling external functions once a full integer has accumulated and/or using higher resolution functions where appropriate).

SCREENSHOTS

If applicable, add screenshots to help explain your problem.

ADDITIONAL CONTEXT

Add any other context about the problem here.

(Please, remember to close the issue when the problem has been addressed)

[github-actions]

This issue has been automatically marked as stale because it hasn't any activity in last few weeks. It will be closed if no further activity occurs. Thank you for your contributions.

[MyriadBugs]

This is still an issue.

[arendst]

I guess the "break" is dependent on the rotary hardware. Both my MiDesk and non-MiDesk rotaries still work fine in both directions with the corresponding hardware option (either using define MI_DESK_LAMP or (no) option A5). The difference in rotary hardware is signalled by this.

I did change both the color overflow and SO43 range issues as you suggested

[MyriadBugs]

Could be some combination of noise and the debounce code, On a re-read the isr makes more sense now, but I could see how it could get locked into one direction or another.