Fix Modbus task scheduling and simplify task handling#3547
Fix Modbus task scheduling and simplify task handling#3547sfeilmeier merged 3 commits intoOpenEMS:developfrom
Conversation
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.
Co-authored-by: Sn0w3y <[email protected]>
926c2d5 to
1cef50f
Compare
da-Kai
left a comment
There was a problem hiding this comment.
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.
|
Hi @da-Kai, thanks for looking into this. The System.out logging has been removed from the tests. |
Codecov Report✅ All modified and coverable lines are covered by tests.
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:
|
sfeilmeier
left a comment
There was a problem hiding this comment.
Thank you for your continued contributions! 🚀
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.