Skip to content

Fix Modbus task scheduling and simplify task handling#3547

Merged
sfeilmeier merged 3 commits intoOpenEMS:developfrom
timo-schlegel:feature/ModbusBridgeFixTasks
Feb 2, 2026
Merged

Fix Modbus task scheduling and simplify task handling#3547
sfeilmeier merged 3 commits intoOpenEMS:developfrom
timo-schlegel:feature/ModbusBridgeFixTasks

Conversation

@timo-schlegel
Copy link
Copy Markdown
Contributor

As discussed and suggested in https://community.openems.io/t/asking-for-feedback-new-modbusworker-implementation/1747/18

The Modbus task handling did not behave as expected because the INITIAL_WAIT task waited for the full planned delay instead of reacting to the EXECUTE_WRITE event. Since it used WaitDelayTask instead of waitMutexTask, it could not be interrupted by the write event. As a result, the planned wait effectively ended after the WRITE event.

This caused write tasks to execute too late, while read tasks were executed immediately after the WRITE event. Consequently, the READ_BEFORE_WRITE and WAIT_FOR_WRITE tasks were never executed.

From a design perspective, the WRITE event should be processed as early as possible after the "execute write" trigger, while READ tasks should finish as late as possible before the "before process image" event. Therefore, the planned waiting time must only occur within the WAIT_BEFORE_READ task, between WRITE and READ. The INITIAL_WAIT task must wait for the EXECUTE_WRITE event instead of a fixed delay.

This commit simplifies the Modbus task handling by removing unused tasks and fixing the INITIAL_WAIT task to use an interruptible wait via waitMutexTask. The associated tests were updated accordingly to ensure consistent and predictable system behavior.

timo-schlegel and others added 2 commits January 29, 2026 21:18
The Modbus task handling did not behave as expected because the
INITIAL_WAIT task waited for the full planned delay instead of reacting
to the EXECUTE_WRITE event. Since it used WaitDelayTask instead of
waitMutexTask, it could not be interrupted by the write event. As a
result, the planned wait effectively ended after the WRITE event.

This caused write tasks to execute too late, while read tasks were
executed immediately after the WRITE event. Consequently, the
READ_BEFORE_WRITE and WAIT_FOR_WRITE tasks were never executed.

From a design perspective, the WRITE event should be processed as early
as possible after the "execute write" trigger, while READ tasks should
finish as late as possible before the "before process image" event.
Therefore, the planned waiting time must only occur within the
WAIT_BEFORE_READ task, between WRITE and READ. The INITIAL_WAIT task
must wait for the EXECUTE_WRITE event instead of a fixed delay.

This commit simplifies the Modbus task handling by removing unused tasks
and fixing the INITIAL_WAIT task to use an interruptible wait via
waitMutexTask. The associated tests were updated accordingly to ensure
consistent and predictable system behavior.
@timo-schlegel timo-schlegel force-pushed the feature/ModbusBridgeFixTasks branch from 926c2d5 to 1cef50f Compare January 30, 2026 13:44
Copy link
Copy Markdown
Contributor

@da-Kai da-Kai left a comment

Choose a reason for hiding this comment

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

Hi @timo-schlegel,

thanks for the PR 👍.

I have skimmed through the community discussion and it looks good at first glance. However, I would need to test it properly before approving.

One thing I noticed is that there are some System.out statements in the tests. Please remove them as unit tests usually do not need logging. They should be expressive through JUnit asserts.

@timo-schlegel
Copy link
Copy Markdown
Contributor Author

Hi @da-Kai,

thanks for looking into this. The System.out logging has been removed from the tests.

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

❗ There is a different number of reports uploaded between BASE (eafccb2) and HEAD (1b78b37). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (eafccb2) HEAD (1b78b37)
java 1 0
Additional details and impacted files
@@              Coverage Diff               @@
##             develop    #3547       +/-   ##
==============================================
- Coverage      59.37%   29.29%   -30.08%     
==============================================
  Files           3068      290     -2778     
  Lines         133081     8298   -124783     
  Branches        9816     1418     -8398     
==============================================
- Hits           79003     2430    -76573     
+ Misses         51099     5736    -45363     
+ Partials        2979      132     -2847     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Contributor

@sfeilmeier sfeilmeier left a comment

Choose a reason for hiding this comment

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

Thank you for your continued contributions! 🚀

@sfeilmeier sfeilmeier merged commit c2f5014 into OpenEMS:develop Feb 2, 2026
5 of 7 checks passed
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.

3 participants