Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughInput and output EPICS CA record builders now add one to string lengths: they use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fastcs/transports/epics/ca/util.py (1)
247-251:⚠️ Potential issue | 🟠 MajorApply the
DEFAULT_STRING_WAVEFORM_LENGTH - 1adjustment to explicit-length strings as well.The implicit-length case (line 251) correctly accounts for EPICS' NUL terminator by subtracting 1, but the explicit-length case (line 249) does not. In
pythonSoftIOC,longStringIn/Out(length=N)setsNELM=N, leaving onlyN-1bytes for usable payload. Currently,String(length=3)truncates to 3 characters but the IOC record can only store 2. Change line 249 toreturn value[: string.length - 1]to keep both paths consistent with the NELM-to-payload semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fastcs/transports/epics/ca/util.py` around lines 247 - 251, In the String handling branch (the case matching String() as string), adjust the explicit-length slicing to account for EPICS NUL terminator semantics the same way the implicit-length path does: replace the current slice using string.length with one using string.length - 1 so explicit longStringIn/Out(length=N) yields at most N-1 usable characters; keep the implicit branch using DEFAULT_STRING_WAVEFORM_LENGTH - 1 unchanged so both paths use NELM-to-payload semantics consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/fastcs/transports/epics/ca/util.py`:
- Around line 247-251: In the String handling branch (the case matching String()
as string), adjust the explicit-length slicing to account for EPICS NUL
terminator semantics the same way the implicit-length path does: replace the
current slice using string.length with one using string.length - 1 so explicit
longStringIn/Out(length=N) yields at most N-1 usable characters; keep the
implicit branch using DEFAULT_STRING_WAVEFORM_LENGTH - 1 unchanged so both paths
use NELM-to-payload semantics consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bd23e02-ebe5-4b15-8a9d-9cc0fd5763a6
📒 Files selected for processing (2)
src/fastcs/transports/epics/ca/util.pytests/transports/epics/ca/test_ca_util.py
GDYendell
force-pushed
the
epics-ca-string-truncation
branch
2 times, most recently
from
76516a2 to
ba10651
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/transports/epics/ca/test_softioc.py (1)
69-76:⚠️ Potential issue | 🟡 MinorCover the remaining string-length branches.
Line 75 now checks the default
String()path, but this PR’s production change also affects theString(length=...)branch in_make_in_recordand the matching_make_out_recordpath. As-is, either of those can regress without this suite failing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/transports/epics/ca/test_softioc.py` around lines 69 - 76, Add parametrize cases to cover the String(length=...) branches so the tests exercise both _make_in_record and _make_out_record logic for non-default lengths; specifically add entries using AttrR(String(length=257)) and another for a shorter length (e.g., 256) with appropriate record_type ("longStringIn"/"longStringOut") and kwargs mirroring the existing case (length, DESC, initial_value) so the tests assert behavior for the explicit-length path in _make_in_record and the corresponding _make_out_record handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/transports/epics/ca/test_softioc.py`:
- Around line 69-76: Add parametrize cases to cover the String(length=...)
branches so the tests exercise both _make_in_record and _make_out_record logic
for non-default lengths; specifically add entries using
AttrR(String(length=257)) and another for a shorter length (e.g., 256) with
appropriate record_type ("longStringIn"/"longStringOut") and kwargs mirroring
the existing case (length, DESC, initial_value) so the tests assert behavior for
the explicit-length path in _make_in_record and the corresponding
_make_out_record handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0d2de550-5757-49c8-afd3-b5bae3de5476
📒 Files selected for processing (2)
src/fastcs/transports/epics/ca/util.pytests/transports/epics/ca/test_softioc.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fastcs/transports/epics/ca/util.py
This accounts for the null terminator added when storing to the record
GDYendell
force-pushed
the
epics-ca-string-truncation
branch
from
ba10651 to
68cadb5
Compare
2 participants
This accounts for a null terminator at the end of the string in the record.
Summary by CodeRabbit