Fix frontend handling of rotary encoder changes with ValueClicker

[rohanpurohit]

This PR fixes #356 Regression from #296

Set Value Modal in multistep popup wasn't affected by this regression as it has its own local function to update the rotary data that did not have the value dependency in useCallback hook

[rohanpurohit]

@ethanjli I have tested this on my end with the rotary encoder, and it works on light testing! I updated the issue to reflect that it was buggy on all ValueClickers except alarm modal which was natural as the bug was in RotaryEncoderController. Thanks!

[ethanjli]

Yup, it seems to work on my end in light testing, too! Previously it seemed to be working fine in the "Set Value" modal, is that because that component doesn't use RotaryEncoderController?

[rohanpurohit]

@ethanjli SetValueContent in multiStepWizard/multistepPopup which was working has it's own local RotaryEncoderController in the component itself, where value in this case is a local state initialized with commitedSetting that comes from the multistepwizard (see here) and the callback function does not have the value parameter as a dependency here, Thus this PR is consistent with that, I guess we should reuse the RotaryEncoderController from controllers, I don't see any reason why we cannot do that. This I guess can be done with other refactoring mentioned in your code comments PR. I will update the PR description with a short summary

[ethanjli]

Ah, ok. I think it would be reasonable to unify the rotary encoder code for SetValueContent with our RotaryEncoderController in either this PR or a future PR, it's up to you.

Do we now have a full explanation for why the callback function breaks if we specify that the value parameter is a dependency for the callback, or is that still being investigated?

[ethanjli]

The latest version works fine for me on the RPi in light testing.

[rohanpurohit]

@ethanjli best explanation for the behavior we are seeing is the components that are passing in the value parameter to the RotaryEncodeController is rendering multiple times, therefore it is passing in value multiple times instead of let's say only when the value actually changes, this behaviour can easily be tested with a console.log for value here, which will result to something like this results

In this log, I'm at the quickstart screen and values for both fio2 and flow are being passed at the same time to the controller, and useCallBack according to the documentation, checks for the dependencies in the array and sends a memoized data back, which is a dependency for useEffect which then keeps running until there's no change, now the value parameter is basically spammed almost every second thus it keeps changing randomly (this would be because the parent component itself is rendering that many times), this also applies to components where there's only one value like SetValueModal, on every re-render value is passed even if there is no change.. so the current value multiple times, and the function keeps re-calculating the newValue parameter I assume.

[ethanjli]

Great detective work! Am I understanding correctly if I interpret this as meaning that there's some sort of circular dependency between the input value and the output newValue (which is output as onClick(newValue)), where calling onClick(newValue) causes value to be treated as updated and thus triggers the callback? If so, I think removing value from the list of callback dependencies is a workaround, and maybe RotaryEncoderController can be made more correct by finding a different way to break the circular dependency. For example:

1. (If this is possible, I think this is preferable) making onClick(newValue) not trigger an "update" to the value variable given as input to this callback, e.g. splitting up some variable in the code using RotaryEncoderController into two separate variables
2. (otherwise) keeping track of the previous value as local state in the controller, and returning early if the new value is the same I haven't done the work to fully understand RotaryEncoderController and how it's used, so I'm not sure if either approach will actually work

[rohanpurohit]

Great detective work! Am I understanding correctly if I interpret this as meaning that there's some sort of circular dependency between the input value and the output newValue (which is output as onClick(newValue)), where calling onClick(newValue) causes value to be treated as updated and thus triggers the callback? If so, I think removing value from the list of callback dependencies is a workaround, and maybe RotaryEncoderController can be made more correct by finding a different way to break the circular dependency. For example:

So I feel there's no circular dependency between the two, RotaryEncoderController or newValue should not depend on value, it's just an initial state to calculate step changes, currently the structure is such that we have a UseEffect that has 2 dependencies updateRotaryData and isActive, where isActive is according to the comment * @prop {boolean} isActive Determines if Rotary encoder should operate, and updateRotaryData is our function when we pass it into the UseEffect the compiler warns us to wrap the function in an UseCallback to avoid infinite re-render, therefore updateRotaryData is only changed when any of the array dependencies change, Hence, I feel value should not be there because updateRotaryData should only care about the step change rather than the value it's passed. I feel it's probably not a workaround and maybe how it should be? I might be wrong.

[ethanjli]

Hmm, I think I understand what you mean - onClick should only be called when rotaryEncoder.step changes, not when value changes. However, leaving value out of the list of dependencies for updateRotaryData fundamentally violates how React designed useCallback to be used - see the note in https://reactjs.org/docs/hooks-reference.html#usecallback that "every value referenced inside the callback should also appear in the dependencies array". But RotaryEncoderController and newValue do depend on value (because newValue is set as value + stepDiff), it's just that we want the updateRotaryData callback to depend on value differently than its other dependencies (i.e. we use value but we don't trigger a call to onClick when it changes). If we use useCallback as it was intended to be used, we need to list all variables it requires, including value. So leaving value off that list is one way of not calling onClick when value changes but rotaryEncoder.step did not change, but it violates the design of useCallback - there should be someway to express this dependency which doesn't go against the design of useCallback, which is why I call the approach of leaving value off the list of dependencies a workaround:

I did a bit of experimentation on updateRotaryData to make changes to rotaryEncoder.step be tracked and updated inside RotaryEncoderController, so that updateRotaryData actually tracks when it's received a change from the rotary encoder and returns early when it hasn't received a change (right now it relies on the less direct approach of adding rotaryEncoder?.step to the array of dependencies and leaving value out of that array). The code for this revised approach needs cleanup (e.g. defining selectors for rotaryEncoder.step and rotaryEncoder.stepDiff instead of getting the entire rotaryEncoder object with the getRotaryEncoder selector and having the entire rotaryEncoder object be a dependency for the callback), but it seems to work correctly:

  const [prevStep, setPrevStep] = React.useState(rotaryEncoder?.step);

  const updateRotaryData = useCallback(
    () => {
      if (!isInitialMount.current) {
        isInitialMount.current = false;
        return;
      }
      if (Number.isNaN(rotaryEncoder?.stepDiff)) {
        return;
      }
      const stepDiff = rotaryEncoder?.stepDiff || 0;
      // console.log(value, rotaryEncoder?.step, prevStep, stepDiff);
      if (rotaryEncoder?.step === prevStep) {
        return;
      }
      setPrevStep(rotaryEncoder?.step);
      const newValue = value + stepDiff;
      if (newValue < min) {
        onClick(min);
      } else if (newValue > max) {
        onClick(max);
      } else {
        onClick(newValue);
      }
      // if (rotaryEncoder.buttonPressed) {
      //   handleConfirm();
      // }
    },
    [rotaryEncoder, min, max, value, prevStep, onClick],
  );

The idea here is that we only cause the onClick side effect when rotaryEncoder.step has actually changed; we still have the dependency on value, but we don't care about value, min, or max when rotaryEncoder.step hasn't changed.

[rohanpurohit]

Hmm, I think I understand what you mean - onClick should only be called when rotaryEncoder.step changes, not when value changes. However, leaving value out of the list of dependencies for updateRotaryData fundamentally violates how React designed useCallback to be used - see the note in https://reactjs.org/docs/hooks-reference.html#usecallback that "every value referenced inside the callback should also appear in the dependencies array". But RotaryEncoderController and newValue do depend on value (because newValue is set as value + stepDiff), it's just that we want the updateRotaryData callback to depend on value differently than its other dependencies (i.e. we use value but we don't trigger a call to onClick when it changes). If we use useCallback as it was intended to be used, we need to list all variables it requires, including value. So leaving value off that list is one way of not calling onClick when value changes but rotaryEncoder.step did not change, but it violates the design of useCallback - there should be someway to express this dependency which doesn't go against the design of useCallback, which is why I call the approach of leaving value off the list of dependencies a workaround:

Yep I agree, in that sense, it is indeed a workaround.

I did a bit of experimentation on updateRotaryData to make changes to rotaryEncoder.step be tracked and updated inside RotaryEncoderController, so that updateRotaryData actually tracks when it's received a change from the rotary encoder and returns early when it hasn't received a change (right now it relies on the less direct approach of adding rotaryEncoder?.step to the array of dependencies and leaving value out of that array). The code for this revised approach needs cleanup (e.g. defining selectors for rotaryEncoder.step and rotaryEncoder.stepDiff instead of getting the entire rotaryEncoder object with the getRotaryEncoder selector and having the entire rotaryEncoder object be a dependency for the callback), but it seems to work correctly:

  const [prevStep, setPrevStep] = React.useState(rotaryEncoder?.step);

  const updateRotaryData = useCallback(
    () => {
      if (!isInitialMount.current) {
        isInitialMount.current = false;
        return;
      }
      if (Number.isNaN(rotaryEncoder?.stepDiff)) {
        return;
      }
      const stepDiff = rotaryEncoder?.stepDiff || 0;
      // console.log(value, rotaryEncoder?.step, prevStep, stepDiff);
      if (rotaryEncoder?.step === prevStep) {
        return;
      }
      setPrevStep(rotaryEncoder?.step);
      const newValue = value + stepDiff;
      if (newValue < min) {
        onClick(min);
      } else if (newValue > max) {
        onClick(max);
      } else {
        onClick(newValue);
      }
      // if (rotaryEncoder.buttonPressed) {
      //   handleConfirm();
      // }
    },
    [rotaryEncoder, min, max, value, prevStep, onClick],
  );

The idea here is that we only cause the onClick side effect when rotaryEncoder.step has actually changed; we still have the dependency on value, but we don't care about value, min, or max when rotaryEncoder.step hasn't changed.

Yes I feel that's a great approach, just tested it on my end as well and it works.

[rohanpurohit]

working fine on my end with the latest changes!

[rohanpurohit]

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.