Prevent the rotary encoder from setting min limit above max limit in the frontend

[rohanpurohit]

This PR adds #374 fix for rotary encoder

[rohanpurohit]

My observations from testing:

• normal rotary encoder movement everything seems fine.
• if the rotary encoder is moved too fast, the value goes beyond the min, max but comes back to the disabled limit, I'm not sure if we can fix this without altering the RotaryEncoderController

[ethanjli]

I'm able to reproduce your observations. However, I also was able to quickly turn the rotary encoder to set min > max, and the min didn't get automatically moved back down to be at max again. Only after I turned the rotary encoder again did the min get moved back down to the max value.

From skimming over the code, I see code adjusting the min/max params passed to ValueClicker based on a disableIncrement/disableDecrement local state. I think the brittle behavior described above is occurring because of this implementation. Do you think we can set the min and max params for ValueClicker without relying on disableIncrement/disableDecrement?

[rohanpurohit]

@ethanjli everything seems to be working fine on my end now with the latest changes. can you confirm the same?

[ethanjli]

It works. I did find a more subtle bug in testing: when adjusting the upper alarm limit, if I rotate the rotary encoder and press the decrement button at nearly the same time, I am able to force a race condition which allows the upper limit to be set to below the lower limit. I'm not always able to make this happen because it depends on the relative timing of events (when the rotary encoder message arrives at the frontend vs. when the decrement button is pressed vs. when the decrement button becomes disabled - usually I have to turn the dial and then after a very short moment then press the decrement button), but I'm able to do it often enough. I think we do need to fix the logic related to handling of increment/decrement buttons so that the min and max ValueClicker limits are always enforced on them independent of whether the buttons are disabled; we could do that in this PR (since it's so small right now) or a future PR (if that will be more work than just a small hotfix) - the proper solution for handling of increment & decrement buttons will be such that even if we never disable the buttons, pressing them when lower = upper will not allow lower > upper. Do you want to fix that in this PR or separately?

[rohanpurohit]

It works. I did find a more subtle bug in testing: when adjusting the upper alarm limit, if I rotate the rotary encoder and press the decrement button at nearly the same time, I am able to force a race condition which allows the upper limit to be set to below the lower limit. I'm not always able to make this happen because it depends on the relative timing of events (when the rotary encoder message arrives at the frontend vs. when the decrement button is pressed vs. when the decrement button becomes disabled - usually I have to turn the dial and then after a very short moment then press the decrement button), but I'm able to do it often enough. I think we do need to fix the logic related to handling of increment/decrement buttons so that the min and max ValueClicker limits are always enforced on them independent of whether the buttons are disabled; we could do that in this PR (since it's so small right now) or a future PR (if that will be more work than just a small hotfix) - the proper solution for handling of increment & decrement buttons will be such that even if we never disable the buttons, pressing them when lower = upper will not allow lower > upper. Do you want to fix that in this PR or separately?

I was able to reproduce the regression, I think it's best if we fix it in this PR, as you said it's too small currently, and finish it by the end of 0.12 milestone

the proper solution for handling of increment & decrement buttons will be such that even if we never disable the buttons, pressing them when lower = upper will not allow lower > upper

I will follow this approach.

[rohanpurohit]

I tested this two ways:

1. On the Raspberry Pi, trying to reproduce the previous behavior. I have not been able to reproduce the behavior.
2. On my computer commenting out the disableIncrement and disableDecrement code and confirming that even when lower == upper, when I press the increment and decrement buttons I am never able to reach lower > upper. This works.

I also see that in ValueClicker, AlarmsPage, and AlarmModal, the disableIncrement and disableDecrement variables are now only used for setting the enable/disable state of those buttons, and not for anything else. So I think the issue is fixed.

From looking at those three files, I see that each of them makes its own local useState for disableIncrement and disableDecrement, and each has its own useEffect for setting those states; and both AlarmsPage and AlarmModal have their own sections in their returned components making grid containers of ValueClicker pairs for increment/decrement and slider and label display. Is there any way we can reduce the duplication of states and/or code, either in this PR or a future PR?

Yeah since we are now passing min and max dynamically disableIncrement and disableDecrement are redundant, I will clean it up in this PR.

[rohanpurohit]

@ethanjli since I added the fix for #386 in this PR, that would need a confirmation from you, and light testing after cleanup. seems to be working fine on my end.

1. This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
2. Were any of these contributions also part of work you did for an employer or a client? No.
3. Does this work include, or is it based on, any third-party work which you did not create? No.