Use integer division in encode_phys to prevent rounding errors with int64

[yves-chevallier]

While testing int64 values with SDOs, I observed an unexpected behavior:

>>> value = 0x55554444AAAABBBB
>>> d.node.sdo[16384][4].phys = value
>>> hex(d.node.sdo[16384][4].phys)
'0x55554444aaaabc00'

Upon investigation, it appears that the issue originates from the encode_phys function:

>>> value = 0x55554444AAAABBBB
>>> sdo.od.factor
1
>>> hex(sdo.od.encode_phys(value))
'0x55554444aaaabc00'

The problem lies in the fact that the division is performed using floating-point arithmetic rather than integer arithmetic, leading to rounding that causes a loss of up to 10 bits of precision.

def encode_phys(self, value: Union[int, bool, float, str, bytes]) -> int:
    if self.data_type in INTEGER_TYPES:
        value /= self.factor
        value = int(round(value))
    return value

To address this, we should detect whether the input value is an integer and, if so, perform integer division (//) instead of floating-point division. Additionally, it may be prudent to emit a warning when factor is not an integer, as this could lead to precision loss.

[acolomb]

I'm not convinved this is the right approach. Truncating may be worse than rounding in some existing use cases, so we should avoid breaking them / changing to different values unexpectedly. OTOH, you can always do the calculation yourself and assign to .raw instead. Which might be better for just the few cases where large integers cause inaccuracy.

[acolomb]

round() already returns an integer if ndigits is omitted.

[yves-chevallier]

Round was the original code. My contribution is only the // division, which is my use case is the right thing to do. What do you suggest?

[acolomb]

If you touch / edit this code and such a useless call is found, please remove it as part of your change.

[sveinse]

The problem lies in the fact that the division is performed using floating-point arithmetic rather than integer arithmetic, leading to rounding that causes a loss of up to 10 bits of precision.

I think TS has a point here. It should be possible to reliably pass int64 through the encode function.

To address this, we should detect whether the input value is an integer and, if so, perform integer division (//) instead of floating-point division.

This could be a good solution since 64-bit double can only hold up to 52 bits (+ sign bit) in the mantissa, so any values higher than that will get truncated in the float division.

Additionally, it may be prudent to emit a warning when factor is not an integer, as this could lead to precision loss.

Is it a use case to have non-integer factors? When do we get into this use case? Does the standard permit float scale factors?

Another alternative would be to skip the division altogether if the factor is 1. In most cases factor is not used at all, and I'd like to suggest that int64 with scale factor other than 1 is somewhat uncommon use case.

[acolomb]

I'm okay with bypassing the division altogether if the factor is one, as in int(1). That is also the default value and if no conversion is needed, I'd recommend to just use .raw instead of .phys.

Is it a use case to have non-integer factors? When do we get into this use case? Does the standard permit float scale factors?

For example we want a quantity expressed as a percentage 0.0 ... 100.0 , but the bus representation is 0 ... 65535. I'd put in .factor = 100 / 65535. The CiA 306 standard does not mention the unit and factor attributes at all AFAIK, those are extensions specific to this library (and adopted by some other implementations).

I see it as a breaking change if suddenly integer division is applied for a perfectly valid use-case that previously returned a properly rounded value. An integer factor can only upscale, such as mapping a quantity in meters into a common range with other variables that work with millimeters: .factor = 1000. When I want to send back such a value, which might have higher precision from an internal calculation, this PR changes what is sent: 5555 mm // 1000 mm/m = 5 m, where previously it would have resulted in round(5555 mm / 1000 mm/m) = round(5.555 m) = 6 m.

That's a subtle difference but could change some systems' behavior in non-trivial and hard to detect ways. Pretty high risk for fixing an edge case which should better be avoided altogether by doing the calculation as needed outside the library, then using the .raw member. At least I'd want some unit tests showing the current results and making sure they don't change inadvertently for common cases. That documents clearly what to expect from the conversion in edge cases and could also serve to show the problem at hand.

By the way, if I read this code (current or from the PR) correctly, then we don't even apply the conversion if the OD variable is already a float?

[sveinse]

I'm okay with bypassing the division altogether if the factor is one, as in int(1). That is also the default value and if no conversion is needed, I'd recommend to just use .raw instead of .phys.

I still would expect .phys with scale factor 1 to not have side-effects.

I see it as a breaking change if suddenly integer division is applied for a perfectly valid use-case that previously returned a properly rounded value. An integer factor can only upscale, such as mapping a quantity in meters into a common range with other variables that work with millimeters: .factor = 1000. When I want to send back such a value, which might have higher precision from an internal calculation, this PR changes what is sent: 5555 mm // 1000 mm/m = 5 m, where previously it would have resulted in round(5555 mm / 1000 mm/m) = round(5.555 m) = 6 m.

A simple fix is to use integer division only if both operands are int and the dividend is not fully representable in float.