Update Characteristic value when setProps called

[Shaquu]

Update Characteristic value when setProps called

♻️ Current situation

This PR is mainly about number values.
Currently, Characteristic default value is based upon minValue.
Once setProps is called and minValue is changed, then also value should be changed accordingly.

💡 Proposed solution

public setProps(props: Partial<CharacteristicProps>): Characteristic {
  //...

  if (this.props.format) {
    switch (this.props.format) {
      case Formats.BOOL:
      case Formats.STRING:
      case Formats.DATA:
      case Formats.TLV8:
      case Formats.DICTIONARY:
      case Formats.ARRAY:
        break;
      default:
        if (this.value) {
          const minValue = this.props.minValue ?? 0
          const maxValue = this.props.maxValue ?? minValue
  
          if (this.value < minValue) {
            this.updateValue(minValue)
          } else if (this.value > maxValue) {
            this.updateValue(maxValue)
          }
        }
    }
  }	 

  return this;
}

Problem that is solved

This PR will clamp Characteristic value, so there is no situation when allowed value range is 10-20 and value is 0.

Implications

updateValue is called, which means some actions might be triggered

Testing

Tested locally before and after with success.

Reviewer Nudging

Right at the top :)

[Supereg]

Thanks for the PR. Great you caught that one 🚀

My comments are mainly to direct you to reuse some of the already existent logic covering cases like that. Additionally, solely checking for the minValue and maxValue properties might not be enough to fully address the issue.

[Shaquu]

@Supereg I will have a look at this soon, thanks for comments!

[Shaquu]

Can you have a look @Supereg?
Also looks like there are some issues with tests on beta branch.

[Supereg]

Hey sorry for the delay, I wasn't sure if a follow-up review was already requested, and then kinda forgot about this 😕

Thanks for incorporating my feedback. I had some concerns on how setProps is used internally, as it is partly called in yet uninitialized state. Happy to year your points in that discussion :)

In regard to the tests, quickly scanned over it. Seems like, due to the added validateUserInput call (probably also because it is called through the constructor) the expected test results do not align anymore. Specifically the expectations on how many calls were made to characteristicWarning are now a bit off. Would be great if you could investigate those cases, and adjust the values if it makes sense.

[coveralls]

Pull Request Test Coverage Report for Build 3247995066

• 9 of 10 (90.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.06%) to 51.788%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/lib/Characteristic.ts	9	10	90.0%

Totals
Change from base Build 3095726760:	0.06%
Covered Lines:	5724
Relevant Lines:	10098

---

💛 - Coveralls

[Shaquu]

@Supereg can I ask for hints or comments?

[Shaquu]

@Supereg bump

[Supereg]

Looks good, thanks for the contribution 🚀

[ebaauw]

Did you guys check this change for Programmable Switch Event and other eventOnly characteristics, if there are any? I fear this might cause a ghost button action.

I now get the “characteristic was supplied an illegal value” on restart, when having changed the supported button actions, and the last button action before the restart is no longer supported. I thought about changing the characteristic value when changing the properties, but don’t know how to do that without generating a ghost button action.

[Supereg]

My intention was to add some test cases in a follow up PR. Thanks for the input 👍

I now get the “characteristic was supplied an illegal value” on restart, when having changed the supported button actions, and the last button action before the restart is no longer supported.

You are justing calling setProps on a ProgrammabelSwitchEvent characteristic? Or am I misunderstanding something?

[ebaauw]

Yes. Based on the config.json settings, a button might support Double Press and/or Long Press in addition to Single Press. E.g. the doublePressTimeout and longPressTimeout settings in Homebridge RPi, or the hueDimmerRepeat setting in Homebridge Hue.

On restart, Homebridge restores the Stateless Programmable Switch from cache, including the last button action as the value of Programmable Switch Event. My plugin sets the characteristic properties based on the changed config.json settings. When the last button action from before the change is now no longer supported, Homebridge complains that the (restored) value is out of bound.

I think for eventOnly characteristics, there should be no check on setProps(), only on setValue(), updateValue(), and equivalents. HomeKit won't ever read the characteristic, so it won't see the out of bound value anyways.