homieiot / convention

🏡 The Homie Convention: a lightweight MQTT convention for the IoT
https://homieiot.github.io/
Other
714 stars 59 forks source link

Unexpected behavior for step rounding according to spec in Integer/Float ranges when `min` or `max` are missing #301

Open schaze opened 1 week ago

schaze commented 1 week ago

Description:

The current spec for handling integer/float ranges with steps, [min]:[max][:step], specifies that the base for calculating a proper value based on the step should be min, max, or the current property value (in that order). However, the behavior when either min or max is missing results in different rounding behavior that is not explicitly clarified in the spec.

Specifically, the behavior of rounding changes depending on whether the base for step rounding is derived from the min or max value. This leads to different rounding results in cases such as 0:10:2 vs :10:2, where:

This results in inconsistencies that may not be immediately obvious from reading the spec.

Examples:

  1. Example 1: Range 0:10:2

    • This range specifies a minimum of 0, a maximum of 10, and a step of 2. The base for rounding is min (0), so rounding occurs upwards from the base.
    • Rounding behavior:
    Value Rounded Value Result
    0 0 Success
    1 2 Success
    2 2 Success
    3 4 Success
    4 4 Success
    5 6 Success
    6 6 Success
    7 8 Success
    8 8 Success
    9 10 Success
    10 10 Success
    11 12 - Out of range Error
  2. Example 2: Range :10:2

    • This range specifies no lower bound, a maximum of 10, and a step of 2. The base for rounding is max (10), which causes rounding to occur downwards from the maximum value.
    • Rounding behavior:
    Value Rounded Value Result
    0 0 Success (rounded down from max)
    1 0 Success (rounded down from max)
    2 2 Success
    3 2 Success
    4 4 Success
    5 4 Success
    6 6 Success
    7 6 Success
    8 8 Success
    9 8 Success (rounded down from max)
    10 10 Success
    11 12 - Out of range Error

Issue:

The spec does not clearly indicate the expected behavior when rounding from max vs. min. While the spec indicates that the base for step calculation should be either min, max, or the current property value (in that order), it is not clear that the rounding behavior would differ significantly depending on which base is used. This could lead to confusion for implementers who expect consistent rounding behavior regardless of whether min or max is specified.

Request:

We should improve the spec by:

  1. Explicitly stating how rounding behavior changes based on whether min or max is used as the base for step calculation.
  2. Providing examples to demonstrate how values are rounded when min is missing (i.e., rounding down from max).

This clarification would help ensure consistent understanding of how the spec should be implemented and what behavior is expected.

Or think about if we want to adjust the spec to ensure the rounding results stay consistent in both cases.

Tieske commented 1 week ago

Nice catch.

The 2nd example is mathematically correct, but counter intuitive imho. So we should specify how it should round in order to result in the intuitive value.

Tieske commented 1 week ago

base should be calculated like this I think:

base = 0
if mimimum != null then
  base = minimum
elif maximum != null then
  base = maximum
else
  base = current_value
end

// reduce base to below input value, to have min+max behave the same
if base > input_value then
  base = round((input_value - base) / step - 1, 0) * step + base
end
Tieske commented 1 week ago

Wondering; should we be creating a generic test suite for all sorts of validations? That would ease writing homie libraries quite a bit I think

similar to these json schema ones: https://github.com/json-schema-org/JSON-Schema-Test-Suite/tree/main/tests/draft4

schaze commented 1 week ago

I actually stumbled across this while writing extensive tests for my rust library. It provides a fully compliant parser for the formats and value types as well as validation of data input according to the property description. Maybe we could use this already, but I am not sure what exactly you have in mind (JSON Schema validation will most likely not be able to cover all our formatting rules...).

Tieske commented 1 week ago

sorry should have been more clear. Intent is not to use JSON schema for validation, but writing test cases in JSON, like this one: https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/main/tests/draft4/anyOf.json it is a file that lists all the test cases, and expected results for the JSONschema "anyOf" operator.

So instead of defining all the testcases yourself, you parse the JSON file, and in a loop run all the tests contained within it.

Example: This module; https://github.com/Tieske/lua-resty-ljsonschema/tree/master/spec contains the repo above as a submodule. In the test suite tyhe files are loaded, and tests executed: https://github.com/Tieske/lua-resty-ljsonschema/blob/master/spec/suite_spec.lua

Tieske commented 1 week ago

we could create one centralized set of tests, here in the homiet github org, and every library can run tests against those sets. That would cover your comment above:

This could lead to confusion for implementers who expect consistent rounding behavior regardless of whether min or max is specified.

schaze commented 6 days ago

Ah now I get it. Yes I think that is a great idea!

Tieske commented 6 days ago

first attempt here: https://github.com/homieiot/homie-testsuite/blob/main/homie5/formats/boolean.yml

please check, then we can add the numeric stuff as well

PS. I used yaml instead of json, because it supports comments

schaze commented 4 days ago

I would prefer having the defining properties of the test under one attribute so we can directly parse it in as the object to be tested where applicable:

tests:
  - description: proper format is accepted
    property:
      type: boolean
      format: 'off,on'
    valid: true

This way I could parse the property description directly into the type used in my library also using the same mechanism as I do for the real thing instead of manually constructing a property description.

To make it more generic we could change it to:

tests:
  - description: proper format is accepted
    test_definition:
      type: boolean
      format: 'off,on'
    input_data: {}
    valid: true

naming is as always hard, so test_definition and input _data we're just the first thing that came to mind.

Tieske commented 4 days ago

that's the part I struggled with. Changing it like that is fine by me

schaze commented 4 days ago

Let me try actually implementing this for my library before we do more definitions to see if it works out as expected. I should have some time on Sunday night.

schaze commented 3 days ago

@Tieske : I created a pull request to your test repo. Please have a look and let me know what you think. https://github.com/homieiot/homie-testsuite/pull/1