Skip to content

Conversation

@GDYendell
Copy link
Contributor

@GDYendell GDYendell commented Jan 30, 2026

Summary by CodeRabbit

  • Bug Fixes

    • EPICS CA transport now enforces 1D-only waveforms: multi-dimensional waveforms are skipped and a warning is logged, preventing unsupported PV creation and avoiding runtime issues.
    • Improved warning messages for name-length issues to surface potential problems earlier.
  • Tests

    • Added a test confirming only 1D waveforms are processed for PV creation and multi-dimensional waveforms are discarded.

@codecov
Copy link

codecov bot commented Jan 30, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.84%. Comparing base (9967981) to head (2a9cf2f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   90.82%   90.84%   +0.01%     
==========================================
  Files          70       70              
  Lines        2551     2555       +4     
==========================================
+ Hits         2317     2321       +4     
  Misses        234      234              

☔ 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.

@coderabbitai
Copy link

coderabbitai bot commented Feb 11, 2026

📝 Walkthrough

Walkthrough

EPICS CA IOC now skips Waveform attributes with dimensionality >1 (logs a warning) and computes PV names only after this check. Some print statements were replaced with logger.warning. A unit test was added to assert only 1D Waveforms produce PV creation calls.

Changes

Cohort / File(s) Summary
Waveform Dimension Filtering & PV creation
src/fastcs/transports/epics/ca/ioc.py
Imported Waveform. In _create_and_link_attribute_pvs, added guard: if attribute.datatype is Waveform and len(attribute.shape) > 1, log a warning and continue. Moved PV name computation after this check to avoid creating PVs for non-1D waveforms. Replaced some print calls with logger.warning for name-length warnings.
Waveform Filtering Test
tests/transports/epics/ca/test_softioc.py
Added test_non_1d_waveforms_discarded (appears twice) that builds 0D/1D/2D/3D Waveform attributes, patches _create_and_link_read_pv, instantiates EpicsCAIOC, and asserts only the 1D waveform triggered a PV creation call.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I nibble shapes both big and small,
1D stays cozy, the rest I stall.
A gentle warning, soft and clear,
I skip the grids that won't appear.
Hooray — the PVs now know who to call.

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding warning messages for unsupported (non-1D) Waveform attributes in EPICS CA transport.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch waveform-warnings

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
src/fastcs/transports/epics/ca/ioc.py (2)

135-143: Set attribute.enabled = False for consistency with other skip paths.

The condition is now correct (!= 1), but skipped waveform attributes aren't marked disabled unlike other skip paths in this function (lines 148 and 164 both set attribute.enabled = False). This inconsistency could cause issues if downstream code relies on enabled to determine which attributes were successfully exposed.

♻️ Proposed fix
             if (
                 isinstance(attribute.datatype, Waveform)
                 and len(attribute.datatype.shape) != 1
             ):
                 logger.warning(
                     "Only 1D Waveform attributes are supported in EPICS CA transport",
                     attribute=attribute,
                 )
+                attribute.enabled = False
                 continue

260-263: Remaining print statement should use logger.warning for consistency.

This warning message uses print while the equivalent warnings in _create_and_link_attribute_pvs (lines 149, 159) now use logger.warning. Consider updating for consistent logging.

♻️ Proposed fix
             if len(f"{pv_prefix}:{pv_name}") > EPICS_MAX_NAME_LENGTH:
-                print(
+                logger.warning(
                     f"Not creating PV for {attr_name} as full name would exceed"
                     f" {EPICS_MAX_NAME_LENGTH} characters"
                 )
                 method.enabled = False

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/fastcs/transports/epics/ca/ioc.py`:
- Around line 135-143: The current guard only skips Waveform attributes with
len(attribute.datatype.shape) > 1 and doesn’t mark them disabled; change the
condition to treat any non‑1D waveform as unsupported by using
len(attribute.datatype.shape) != 1, and when that condition matches set
attribute.enabled = False before continuing; keep the logger.warning call (e.g.,
the existing logger.warning message about 1D Waveform attributes) so the
behavior in this EPICS CA transport branch is consistent with other skip paths
(refer to Waveform, attribute.enabled, and logger.warning in the ioc handling
code).

Copy link
Contributor

@ajgdls ajgdls left a comment

Choose a reason for hiding this comment

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

Not directly related to this change, but there are some print statements that should probably be updated to log messages.

Update other related checks to use logger.warning
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.

2 participants