Fixed a regression where floats were rounded incorrectly when no minValue was provided

[Supereg]

♻️ Current situation

The release of v0.10.3 introduced fixes to the float rounding mechanism (see #956, #958). It introduced a regression when no minValue was provided (see #971).

💡 Proposed solution

This PR introduces a fix for this regression.

⚙️ Release Notes

• Fixed a regression where floats were rounded incorrectly when no minValue was provided

➕ Additional Information

This regression slipped through in this review step: https://github.com/homebridge/HAP-NodeJS/pull/958/files#r893353790.

Testing

A regression test was added, covering this case.

Reviewer Nudging

--

[coveralls]

Pull Request Test Coverage Report for Build 3081596305

• 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.007%) to 51.764%

---

Totals
Change from base Build 3081583937:	0.007%
Covered Lines:	5718
Relevant Lines:	10090

---

💛 - Coveralls

[Supereg]

CC: @OrigamiDream

[OrigamiDream]

In my opinion, minValue and maxValue must be defined in characteristic level.

Current state:

const numericMin = maxWithUndefined(this.props.minValue, numericLowerBound(this.props.format));
const numericMax = minWithUndefined(this.props.maxValue, numericUpperBound(this.props.format));

And new suggestion:

/* Needs to find original props */
const originalMin = originalProps.minValue; // if there is no defined default values, code must be failed.
const originalMax = originalProps.maxValue;
const numericMin = Math.max(numericLowerBound(this.props.format), maxWithUndefined(this.props.minValue, originalMin));
const numericMax = Math.min(numericUpperBound(this.props.format), maxWithUndefined(this.props.maxValue, originalMax));

This is not only preventing setting values that exceeding their limits, but also falling back to specific reasonable default value when the developer haven't set its specific value.
No other changes needed except finding original properties.

All characteristic have their min and max values in their definitions.
But what if developers ignore those values on specific purpose?
The code would be failed because it is not correct operation.
Therefore, those values must be required, not optional even custom characteristic.

We have to restrict this to developers, not avoiding faced problem.

[ebaauw]

In my opinion, minValue and maxValue must be defined in characteristic level.

I don’t disagree, but then please issue a warning that these are missing instead.

[ebaauw]

Also happy to see a warning when minValue and maxValue are missing, and leave the 0.10.3 logic.

I do remember issues with setting minValue for temperature to -273.15, because the minStep 0.1 would indeed start there, and no longer accept 20.5.

[Supereg]

I do remember issues with setting minValue for temperature to -273.15, because the minStep 0.1 would indeed start there, and no longer accept 20.5.

That's correct behaviour then.

What I'm proposing is the following logic:

1. minStep is always (by specification) relative to the minValue. Meaning @ebaauw's example I cited above is correctly rejected.
2. When the developer doesn't specify a minValue, we want a) to allow a value in the whole number space (e.g. choosing the lowest possible value for the data type) but b) want to use 0 as the reference point for minStep (this feels the most natural and what developers would expect if they set no value restrictions).

Requiring the minValue to be set is a regression as a) the change wasn't described in the previous PR b) there is no check if minValue is set, no warnings and no documentation about it (we even have fallback code for other things not requiring minValue to be set) and c) this change isn't allowed from a SemVer perspective.

Therefore, I would propose to merge this PR as is for a 0.10.4 release. We could open a discussion separately if we should change the behaviour for a future minor or major version increment of HAP-NodeJS.

[OrigamiDream]

If this PR doesn't break any logics from 0.10.3 when minValue is specified, this is good to be merged for hot fix purpose.
And well, I had mentioned about this problem when my previous PR have been reviewed.

• Fix rounding algorithm #958 (comment)

I would expect minValue will be required props in 0.11.0 or 1.0.0.

[Supereg]

If this PR doesn't break any logics from 0.10.3 when minValue is specified, this is good to be merged for hot fix purpose.

This PR only changes the logic in the case where minValue isn't present. So it doesn't alter any behaviour corrected in any of the previous PPs.