Skip to content

Fix silent fixed-point overflow (ESROGUE-735)#1166

Merged
ruck314 merged 8 commits into
pre-releasefrom
ESROGUE-735
Apr 8, 2026
Merged

Fix silent fixed-point overflow (ESROGUE-735)#1166
ruck314 merged 8 commits into
pre-releasefrom
ESROGUE-735

Conversation

@ruck314

@ruck314 ruck314 commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixed-point variables (Fixed/UFixed) now raise a GeneralError when set to a value outside the representable range, instead of silently clipping or storing an incorrect value
  • Added minValue()/maxValue() to Fixed and UFixed Python model classes so the existing C++ range check in Block::setFixed() is activated
  • Replaced the silent positive-edge-case clipping (fPoint -= 1) in C++ with an explicit overflow check that throws with the variable name, attempted value, and valid range

Test plan

  • Existing 103 core tests pass
  • New test_fixed_point_overflow_raises_error covers: positive overflow, negative overflow, unsigned overflow, negative-into-unsigned, and boundary values
  • Verified with Fixed(16, 14) (the etaQ/etaI register scenario, range +-2.0) that overflow now produces a clear error message

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.
@ruck314 ruck314 requested review from bengineerd and tristpinsm April 4, 2026 03:43
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.61%. Comparing base (b89f702) to head (6bce77f).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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() to Fixed and UFixed Python 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.

Comment thread src/rogue/interfaces/memory/Block.cpp Outdated
Comment thread src/rogue/interfaces/memory/Block.cpp Outdated
Comment thread python/pyrogue/_Model.py
Comment thread tests/core/test_model_variables.py Outdated
Comment thread tests/core/test_model_variables.py
- 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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread src/rogue/interfaces/memory/Block.cpp Outdated
When valueBits_ == 64, casting 1ULL<<63 to int64_t gives INT64_MIN and
negating it is undefined behavior. Derive minInt from maxInt instead.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread python/pyrogue/_Model.py Outdated
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 tristpinsm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks great! One more comment that might need addressing

Comment thread src/rogue/interfaces/memory/Variable.cpp
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.
@ruck314 ruck314 requested a review from tristpinsm April 8, 2026 17:00
ruck314 added 3 commits April 8, 2026 10:09
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.
@ruck314 ruck314 merged commit 5065b0b into pre-release Apr 8, 2026
7 checks passed
@ruck314 ruck314 deleted the ESROGUE-735 branch April 8, 2026 17:28
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