Skip to content

Conversation

@mateuszkobak
Copy link
Contributor

@mateuszkobak mateuszkobak commented Sep 11, 2025

Modbus RTU master (client) library.

Description

Not all functionalities are implemented yet, but things that were needed for now should be there.

Motivation and Context

This was developed primarily for the use in Cognit Project.
See: https://github.com/SovereignEdgeEU-COGNIT/use-case-3-phoenix-demo

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

  • Already covered by automatic testing.
  • New test added: (add PR link here).
  • Tested by hand on: ia32-qemu, imxrt1170

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing linter checks and tests passed.
  • My changes generate no new compilation warnings for any of the targets.

Special treatment

  • This PR needs additional PRs to work (list the PRs, preferably in merge-order).
  • I will merge this PR by myself when appropriate.

@github-actions
Copy link

github-actions bot commented Sep 11, 2025

Unit Test Results

9 277 tests  +63   8 687 ✅ +62   54m 33s ⏱️ +6s
  545 suites + 9     589 💤 ± 0 
    1 files   ± 0       1 ❌ + 1 

For more details on these failures, see this check.

Results for commit 2fed2d2. ± Comparison against base commit 82ac411.

♻️ This comment has been updated with latest results.

@mateuszkobak mateuszkobak marked this pull request as ready for review September 11, 2025 10:15
@mateuszkobak mateuszkobak requested a review from anglov September 11, 2025 10:15
@anglov anglov requested a review from xvuko September 12, 2025 14:29
Copy link
Member

@anglov anglov left a comment

Choose a reason for hiding this comment

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

I put some early notes that will affect internal API and other small comment. To make a full review, I need dig a little deeper into it.

@mateuszkobak mateuszkobak force-pushed the mateuszkobak/modbus branch 2 times, most recently from 299460b to 23cb8ec Compare September 22, 2025 10:46
Copy link
Member

@anglov anglov left a comment

Choose a reason for hiding this comment

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

I am not convinced about way, how reading buffer works. I think it would be better to firstly read frame, and then do the things (in modbus RTU over serial, 3,5 bytes of no communication means end of frame. In your implementation you can always try to get more data, if you miss something.

On the other hand, it will be armored for bad server implementation, which doesn't respect timeouts. So I am not opposing on current situation for now. For me it's important, that read callback should always read whole frame. The rest are internal API, which can be subject of changes.

However I put some comment, that can armor your solution for communication error, and make this more readable.

@mateuszkobak mateuszkobak force-pushed the mateuszkobak/modbus branch 2 times, most recently from d7e4d9b to e4b1ad2 Compare September 24, 2025 12:03
anglov
anglov previously approved these changes Sep 25, 2025
Copy link
Member

@anglov anglov left a comment

Choose a reason for hiding this comment

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

Fine for me as for initial version. It's not a stable library (API will be subject of small changes when new features will be added), but I am ok to go for it for now.

@mateuszkobak mateuszkobak merged commit 80ed251 into master Sep 30, 2025
37 of 39 checks passed
@mateuszkobak mateuszkobak deleted the mateuszkobak/modbus branch September 30, 2025 09:29
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