Fix silent fixed-point overflow (ESROGUE-735)#1166
Conversation
Fixed and UFixed models were missing minValue()/maxValue() methods, causing the C++ range check in Block::setFixed() to be skipped. Out-of-range values were silently clipped (positive overflow) or stored incorrectly (negative overflow) with no error reported. Add minValue()/maxValue() to Fixed and UFixed Python models so the existing range check fires. Replace the silent positive-edge-case clipping in C++ with an explicit overflow error that reports the variable name, attempted value, and valid range.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## pre-release #1166 +/- ##
===============================================
+ Coverage 54.51% 54.61% +0.09%
===============================================
Files 70 70
Lines 7862 7870 +8
Branches 1179 1179
===============================================
+ Hits 4286 4298 +12
+ Misses 3300 3296 -4
Partials 276 276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to eliminate silent fixed-point overflow behavior by enforcing representable-range checks for fixed-point variables and adding coverage to ensure overflow/underflow raises errors.
Changes:
- Added
minValue()/maxValue()toFixedandUFixedPython model classes to propagate representable range into RemoteVariable metadata. - Updated C++ fixed-point write path (
Block::setFixed) to throw on integer-domain overflow instead of silently adjusting the value. - Added new unit test coverage for fixed-point overflow/underflow behavior and min/max metadata exposure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
tests/core/test_model_variables.py |
Adds assertions for fixed-point min/max metadata and a new overflow/underflow test. |
src/rogue/interfaces/memory/Block.cpp |
Replaces silent fixed-point clipping with an explicit overflow check that throws. |
python/pyrogue/_Model.py |
Introduces representable min/max implementations for Fixed and UFixed models. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Use 1ULL shifts in Block::setFixed to avoid UB at 64-bit widths - Fix UFixed.maxValue() to return the signed-range max that C++ actually supports (the getFixed/setFixed path is always two's-complement, so values above 2^(N-1)-1 are not representable for UFixed) - Update UFixed.maximum test assertion from 4095.9375 to 2047.9375 - Replace try/except with pytest.raises(Exception, match=...) for stricter, idiom-consistent overflow tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When valueBits_ == 64, casting 1ULL<<63 to int64_t gives INT64_MIN and negating it is undefined behavior. Derive minInt from maxInt instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
UFixed previously shared rim::Fixed's modelId and was routed through the signed two's-complement code in Block::setFixed/getFixed, so its actual range was the signed Fixed range rather than the [0, 2^bitSize-1]/2^binPoint range documented in fixed_point_models.rst. Add a dedicated rim::UFixed modelId (0x09) with Block::setUFixed/getUFixed that enforces the unsigned range, rejects negative writes, and reads back without sign extension. setFixedPy/getFixedPy now dispatch on modelId so the numpy/list/scalar paths work for both. UFixed.modelId and UFixed.maxValue() are updated to reflect the real unsigned range, and tests cover the new full-range UFixed behavior.
tristpinsm
left a comment
There was a problem hiding this comment.
Looks great! One more comment that might need addressing
Call Block::getFixed and Block::getUFixed directly in the respective cases instead of relying on the shared getFixed_ function pointer. The fall-through worked because getFixed_ was initialized to &Block::getUFixed for UFixed variables, but the code was misleading to readers.
The read path used plain-int shifts (1 << valueBits_ and 1 << (valueBits_ - 1)), which is undefined behavior for valueBits_ >= 31 and silently broke negative-value round-trips on Fixed variables wider than 31 bits (e.g. Fixed(32, 0), Fixed(64, 0)). Use 1LL shifts and skip the subtraction entirely when valueBits_ == 64 (the stored value already sits in int64_t correctly). Mirrors the write-side UB fix in 21bb136. Add a wide-Fixed round-trip regression test covering Fixed(32, 0), Fixed(40, 8), and Fixed(64, 0), including negative and boundary values. CI previously only round-tripped Fixed(16, 4) / UFixed(16, 4), so the sign-extension path was unexercised. Update fixed_point_models.rst to reflect the split rim.Fixed / rim.UFixed modelIds introduced earlier in this PR.
Add regression tests for two corner cases untouched by the existing fixed-point coverage: 1. Wide UFixed round-trip. Exercises Block::setUFixed / Block::getUFixed at valueBits_ >= 32, including the valueBits_ >= 64 ? UINT64_MAX branch in setUFixed's max-range computation and values above the signed-max boundary that must not be sign-extended on read. 2. Fixed / UFixed list variables (numValues > 1). Exercises the numpy-array and per-element paths in Block::setFixedPy / Block::getFixedPy that dispatch to the signed or unsigned scalar setter/getter via a modelId check. Previously no test used a fixed-point base with numValues > 1, so the dispatch was unverified.
Summary
Fixed/UFixed) now raise aGeneralErrorwhen set to a value outside the representable range, instead of silently clipping or storing an incorrect valueminValue()/maxValue()toFixedandUFixedPython model classes so the existing C++ range check inBlock::setFixed()is activatedfPoint -= 1) in C++ with an explicit overflow check that throws with the variable name, attempted value, and valid rangeTest plan
test_fixed_point_overflow_raises_errorcovers: positive overflow, negative overflow, unsigned overflow, negative-into-unsigned, and boundary valuesFixed(16, 14)(the etaQ/etaI register scenario, range +-2.0) that overflow now produces a clear error message