Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix floating point validation on Characteristic Set #956

Merged
merged 2 commits into from
May 29, 2022
Merged

Fix floating point validation on Characteristic Set #956

merged 2 commits into from
May 29, 2022

Conversation

OrigamiDream
Copy link
Contributor

♻️ Current situation

Related:

💡 Proposed solution

The provided fp value must be same with the characteristic value.

Previously, this didn't work because of remainder between provided value and minimum step size.
Therefore, I suggest a new stable constant, epsilon, which is 1.
This patch fixes the issue by reducing remainder significantly.

The value of the constant is minimum.
It would crash plugins if the value is under 1.

⚙️ Release Notes

If your accessory have specific "level" of state, you can now change the ranges to 0~100%.
This makes your Siri reads the level of state properly.

➕ Additional Information

No additional information needed

Testing

"should validate Formats.FLOAT with precision with minimum steps" have been added in Characteristic.spec.ts#L1097

Reviewer Nudging

You don't need hard mathematical understanding!
You can just test it with eps = 0 or eps = 1.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2401686893

  • 0 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 51.771%

Totals Coverage Status
Change from base Build 2243960438: -0.05%
Covered Lines: 5712
Relevant Lines: 10070

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2401686893

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.05%) to 51.771%

Totals Coverage Status
Change from base Build 2243960438: -0.05%
Covered Lines: 5712
Relevant Lines: 10070

💛 - Coveralls

@Supereg Supereg changed the base branch from master to beta-0.10.3 May 29, 2022 21:48
Copy link
Member

@Supereg Supereg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reporting and fixing this issue 🚀

@Supereg Supereg merged commit 0dd5cb1 into homebridge:beta-0.10.3 May 29, 2022
@adriancable
Copy link
Contributor

@Supereg - see my comments in #955. Unfortunately I think this was merged prematurely. This PR replaces a logic error in the rounding algorithm with another logic error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants