MQTT: fix float format #27046
MQTT: fix float format #27046
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Changing from
%.5gto fixed-point formatting with%.3fand trimming will now turn very small non-zero values (e.g.1e-5) into0, so ifencodeis used beyond energy values you may want to guard with a minimum magnitude or fall back to scientific notation for very small numbers. - Consider extracting the float formatting logic into a small helper (e.g.
formatFloat3with a short comment on the rationale/constraints) so that the precision and rounding behavior is explicit and easier to adjust if requirements change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing from `%.5g` to fixed-point formatting with `%.3f` and trimming will now turn very small non-zero values (e.g. `1e-5`) into `0`, so if `encode` is used beyond energy values you may want to guard with a minimum magnitude or fall back to scientific notation for very small numbers.
- Consider extracting the float formatting logic into a small helper (e.g. `formatFloat3` with a short comment on the rationale/constraints) so that the precision and rounding behavior is explicit and easier to adjust if requirements change.
## Individual Comments
### Comment 1
<location> `server/mqtt_test.go:21-29` </location>
<code_context>
assert.Equal(t, "+Inf", m.encode(math.Inf(0)), "Inf not encoded as string")
}
+func TestMqttEncodeFloat(t *testing.T) {
+ m := &MQTT{}
+ assert.Equal(t, "12345.678", m.encode(12345.678), "large energy value with 3 decimals")
+ assert.Equal(t, "12345.6", m.encode(12345.6), "large energy value with 1 decimal")
+ assert.Equal(t, "12345", m.encode(12345.0), "large energy value without decimals")
+ assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals")
+ assert.Equal(t, "1.2", m.encode(1.2), "value with 1 decimal")
+ assert.Equal(t, "0", m.encode(0.0), "zero")
+ assert.Equal(t, "2500", m.encode(2500.0), "power value without decimals")
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests that explicitly cover rounding at the 4th decimal place to ensure the behavior of the new format is locked in
Current tests only cover values that don’t stress rounding at the 4th decimal. Please add cases like `0.1239` → `"0.124"` and `1.9999` → `"2"` (or whatever is expected) to explicitly assert the rounding behavior of `"%.3f"` and guard against regressions from rounding to truncation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| func TestMqttEncodeFloat(t *testing.T) { | ||
| m := &MQTT{} | ||
| assert.Equal(t, "12345.678", m.encode(12345.678), "large energy value with 3 decimals") | ||
| assert.Equal(t, "12345.6", m.encode(12345.6), "large energy value with 1 decimal") | ||
| assert.Equal(t, "12345", m.encode(12345.0), "large energy value without decimals") | ||
| assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals") | ||
| assert.Equal(t, "1.2", m.encode(1.2), "value with 1 decimal") | ||
| assert.Equal(t, "0", m.encode(0.0), "zero") | ||
| assert.Equal(t, "2500", m.encode(2500.0), "power value without decimals") |
There was a problem hiding this comment.
suggestion (testing): Add tests that explicitly cover rounding at the 4th decimal place to ensure the behavior of the new format is locked in
Current tests only cover values that don’t stress rounding at the 4th decimal. Please add cases like 0.1239 → "0.124" and 1.9999 → "2" (or whatever is expected) to explicitly assert the rounding behavior of "%.3f" and guard against regressions from rounding to truncation.
There was a problem hiding this comment.
Pull request overview
This PR fixes floating point precision issues in MQTT value encoding by changing the format from %.5g (5 significant figures) to %.3f (3 decimal places) with trailing zero trimming. This addresses issue #27045 where large energy values were being incorrectly rounded.
Changes:
- Modified
encode()method inserver/mqtt.goto use fixed-point notation with 3 decimals instead of significant figure notation - Added comprehensive test coverage in
server/mqtt_test.goto verify the new formatting behavior - Trailing zeros and decimal points are automatically trimmed for cleaner output
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| server/mqtt.go | Changed float64 encoding from %.5g to %.3f with trailing zero trimming to preserve precision for large energy values |
| server/mqtt_test.go | Added test cases covering various float formatting scenarios including large values, small values, and zero handling |
| assert.Equal(t, "12345.678", m.encode(12345.678), "large energy value with 3 decimals") | ||
| assert.Equal(t, "12345.6", m.encode(12345.6), "large energy value with 1 decimal") | ||
| assert.Equal(t, "12345", m.encode(12345.0), "large energy value without decimals") | ||
| assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals") |
There was a problem hiding this comment.
The test description says "truncated to 3 decimals" but fmt.Sprintf with %.3f actually rounds to 3 decimal places, not truncates. Consider changing "truncated" to "rounded" in the test description for accuracy. For example, 0.1236 would become "0.124" (rounded up), demonstrating rounding behavior rather than truncation.
| assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals") | |
| assert.Equal(t, "0.123", m.encode(0.123456), "small value rounded to 3 decimals") |
| func TestMqttEncodeFloat(t *testing.T) { | ||
| m := &MQTT{} | ||
| assert.Equal(t, "12345.678", m.encode(12345.678), "large energy value with 3 decimals") | ||
| assert.Equal(t, "12345.6", m.encode(12345.6), "large energy value with 1 decimal") | ||
| assert.Equal(t, "12345", m.encode(12345.0), "large energy value without decimals") | ||
| assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals") | ||
| assert.Equal(t, "1.2", m.encode(1.2), "value with 1 decimal") | ||
| assert.Equal(t, "0", m.encode(0.0), "zero") | ||
| assert.Equal(t, "2500", m.encode(2500.0), "power value without decimals") | ||
| } |
There was a problem hiding this comment.
The test coverage should include negative numbers to ensure the formatting works correctly for negative values (e.g., -12345.678, -0.123). While the implementation should handle them correctly, explicit test coverage would be valuable given that energy and power can be negative (e.g., during feed-in).
| func TestMqttEncodeFloat(t *testing.T) { | ||
| m := &MQTT{} | ||
| assert.Equal(t, "12345.678", m.encode(12345.678), "large energy value with 3 decimals") | ||
| assert.Equal(t, "12345.6", m.encode(12345.6), "large energy value with 1 decimal") | ||
| assert.Equal(t, "12345", m.encode(12345.0), "large energy value without decimals") | ||
| assert.Equal(t, "0.123", m.encode(0.123456), "small value truncated to 3 decimals") | ||
| assert.Equal(t, "1.2", m.encode(1.2), "value with 1 decimal") | ||
| assert.Equal(t, "0", m.encode(0.0), "zero") | ||
| assert.Equal(t, "2500", m.encode(2500.0), "power value without decimals") | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage for very large numbers (e.g., values > 1e6) to verify that the formatting produces reasonable output. The old format used scientific notation for very large values, while the new format will produce fixed-point notation which could result in very long strings for extremely large values. However, this might be acceptable depending on the expected range of energy/power values in the application.
| return val | ||
| case float64: | ||
| return fmt.Sprintf("%.5g", val) | ||
| // format with up to 3 decimals, trim trailing zeros |
There was a problem hiding this comment.
Since this is MQTT and not some kind of frontend display: why do we even need to trim trailing digits? Lets keep it simple.
|
Closed in a7b965f |
AI summary:
What has been changed:
In mqtt.go:
float64values has been changed from%.5g(5 significant digits) to%.3f(3 decimal places) with trailing zeros trimmedIn mqtt_test.go:
Comparison:
%.5g)%.3f)12346❌12345.678✅12346❌12345.6✅123.46123.456✅7.89✅7.89✅2500✅2500✅Exported values now retain up to 3 decimal places and are no longer rounded to whole numbers for large values. The precision is (for energy) accurate to ±1 Wh, which is sufficient even for very large values.
Fix #27045