drivers: modbus_rtu: add initial support (v2) #20386

basilfx · 2024-02-13T22:26:47Z

Contribution description

This PR is a reboot of #13913 that got stale. I cloned it, rebased it onto master, adapted it to latest standards (hopefully) and proposed #19819. But after some actual testing, I decided that both PRs were not really useful, and made a lot of assumptions. For more flexibility, I rewrote most of the code. Some notable improvements:

No more hard-coded addresses. If you request address 1000, the slave does not need to allocate a buffer to hold 1000 registers.
Slave implementation uses a request callback, which makes it much simpler to implement dynamic behavior.
Support for exceptions.
Better documented, consistent naming with Modbus specification.

Contrary to the original PR, I removed a lot of #ifdefs that limited which Modbus functions would be supported. It was hard to manage IMHO.

I also extended the test application to test using a single or multiple devices. An example application that provides a Modbus slave implementation is also provided.

Testing procedure

A test application is provided in tests/drivers/modbus.

There are multiple ways to test this. The easiest is to connect RX/TX of one UART to TX/RX of another UART. That way, Modbus runs over RS-232, which should be sufficient to test majority of the implementation. If you take a board with two additional UARTs, then you can use the loopback mode, which will perform additional assertions by comparing data directly.

Alternatively, connect two or more boards via a RS-485 network. Let one run as master and the other as slaves.

See here for more information.

Issues/PRs references

Fixes #13869.

This adds initial support for Modbus RTU in master and slave mode. It supports the following functions: * MODBUS_FC_READ_COILS * MODBUS_FC_READ_DISCRETE_INPUTS * MODBUS_FC_READ_HOLDING_REGISTERS * MODBUS_FC_READ_INPUT_REGISTERS * MODBUS_FC_WRITE_SINGLE_COIL * MODBUS_FC_WRITE_SINGLE_HOLDING_REGISTER * MODBUS_FC_WRITE_MULTIPLE_COILS * MODBUS_FC_WRITE_MULTIPLE_HOLDING_REGISTERS Code has been structured to make it extensible in the future, but preparing it for other uses than Modbus RTU is not part of this commit.

This application will test the Modbus RTU implementation.

This example will demonstrate how to implement a Modbus slave, that interacts with SAUL.

kfessel · 2024-03-14T13:58:21Z

drivers/include/modbus_rtu.h

+    /**
+     * @brief   RTS pin. @p GPIO_UNDEF if not used.
+     */
+    gpio_t pin_rts;


why dont you use ?

Suggested change

/**

* @brief RTS pin. @p GPIO_UNDEF if not used.

*/

gpio_t pin_rts;

gpio_t pin_rts; /**<< RTS pin. @p GPIO_UNDEF if not used.*/

I will update this. I just re-used what was already provided in the first PR :-)

I have addressed this for modbus_rtu_params_t and modbus_rtu_t. I did drop the @internal annotations, which I could not combine.

MrKevinWeiss

Really nice... I would like to see this in and would provide testing in the near future.

MrKevinWeiss · 2024-03-19T09:41:30Z

drivers/modbus/Kconfig

@@ -0,0 +1,34 @@
+# Copyright (c) 2023-2024 Bas Stottelaar <[email protected]>


We are no longer doing dependency modelling with kconfig... This can be dropped.

Right, so I can just remove the Kconfig file?

Removed the Kconfig file.

riot-ci · 2024-03-19T17:04:08Z

Murdock results

✔️ PASSED

6f6f606 fixup! tests/drivers/modbus: add test application

Success	Failures	Total	Runtime
10067	0	10067	09m:34s

Artifacts

drivers/include/modbus_rtu.h

kfessel · 2024-03-21T11:49:25Z

drivers/include/modbus_rtu.h

+    const modbus_rtu_params_t* params;  /**< device parameters */
+    uint32_t timeout;       /**< amount of time (usec) to wait for a slave to
+                                 begin sending a */
+    uint32_t rx_timeout;    /**< time between two bytes before timeing out */


Suggested change

uint32_t rx_timeout; /**< time between two bytes before timeing out */

uint32_t rx_timeout; /**< timeout between bytes*/

kfessel · 2024-03-21T16:30:44Z

drivers/include/modbus_rtu.h

+    modbus_t dev;           /**< @ref modbus_t base class */
+    const modbus_rtu_params_t* params;  /**< device parameters */
+    uint32_t timeout;       /**< amount of time (usec) to wait for a slave to
+                                 begin sending a */


Suggested change

begin sending a */

begin sending*/

basilfx · 2024-03-25T23:35:55Z

I made some additional changes to the implementation.

I removed message validation when using modbus_rtu_send_request. It is up to the caller to provide a valid message.
I also removed message validation in modbus_rtu_recv_request. Validation should now happen in the callback, because it checks 'application level' properties.
I removed the MODBUS_ERR_EXCEPTION error code: exceptions are a valid responses, so leave it up to the application to handle it.
I added support for zero-copy read requests: in this case, the message will directly point to the internal buffer, which does not need copying.
I added support for multi-slave implementations. Sounds like a lot, but I basically allow the request callback to handle any request message, so it is up to the callback to decide whether or not to send a response. This also allows for broadcasted requests (slave identifier == 0). It does require some additional checks in the callback, but I think its worth the flexibility.

kfessel · 2024-03-26T11:00:13Z

drivers/include/modbus_rtu.h

+    uint32_t timeout;                           /**< amount of time (usec) to wait for a slave to
+                                                     begin sending */
+    uint32_t rx_timeout;                        /**< timeout between two bytes */


can u make them a little more descriptive

like "inter_fame_usec_min" and "byte_usec_max"

is that correct?

maybe its is more like frame_space_rx (1,5 chars ) , frame_space_ tx (3,5 chars)

please check me on this

there also isn't a difference in master vs slave am i wrong?

I have moved timeout to the modbus_rtu_params_t, and renamed rx_timeout to byte_timeout.

rx_timeout is calculated during initialization, to save some computation in hot path (debatable). I could remove it: it is based on baud rate, which was not stored before (it is via params).

Fields inside modbus_rtu_t are now marked as internal (with a comment). Also improved comments.

I think it is OK now.

kfessel · 2024-03-27T15:19:10Z

@derMihai since you worked with modbus would you please take a short look

drivers/modbus/modbus_rtu.c

basilfx requested review from leandrolanzieri, jia200x, MrKevinWeiss, aabadie and MichelRottleuthner as code owners February 13, 2024 22:26

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Feb 13, 2024

basilfx mentioned this pull request Feb 13, 2024

drivers: modbus_rtu: add initial support #19819

Closed

basilfx force-pushed the feature/modbus_rtu_v2 branch from d6f6c6f to 3541174 Compare February 14, 2024 07:21

basilfx added 3 commits February 14, 2024 09:42

tests/drivers/modbus: add test application

b55d9ba

This application will test the Modbus RTU implementation.

examples/modbus_saul: add example application

b8a4dfc

This example will demonstrate how to implement a Modbus slave, that interacts with SAUL.

basilfx force-pushed the feature/modbus_rtu_v2 branch from 3541174 to b8a4dfc Compare February 14, 2024 08:42

kfessel reviewed Mar 14, 2024

View reviewed changes

MrKevinWeiss reviewed Mar 19, 2024

View reviewed changes

MrKevinWeiss added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 19, 2024

kfessel reviewed Mar 19, 2024

View reviewed changes

drivers/include/modbus_rtu.h Show resolved Hide resolved

basilfx added 4 commits March 20, 2024 22:16

fixup! drivers/modbus: add initial support

e786016

fixup! drivers/modbus: add initial support

df17335

fixup! drivers/modbus: add initial support

1b887a1

fixup! drivers/modbus: add initial support

66c1099

github-actions bot removed the Area: Kconfig Area: Kconfig integration label Mar 20, 2024

fixup! examples/modbus_saul: add example application

b6d822f

basilfx force-pushed the feature/modbus_rtu_v2 branch from 4e6bfe2 to b6d822f Compare March 20, 2024 23:37

kfessel reviewed Mar 21, 2024

View reviewed changes

basilfx force-pushed the feature/modbus_rtu_v2 branch from 88d0828 to 59e2a59 Compare March 25, 2024 23:30

basilfx added 3 commits March 26, 2024 00:35

fixup! drivers/modbus: add initial support

51ccede

fixup! examples/modbus_saul: add example application

02a0d5f

fixup! tests/drivers/modbus: add test application

d553305

basilfx force-pushed the feature/modbus_rtu_v2 branch from 59e2a59 to d553305 Compare March 25, 2024 23:35

kfessel reviewed Mar 26, 2024

View reviewed changes

basilfx added 5 commits March 26, 2024 17:29

fixup! drivers/modbus: add initial support

e117b49

fixup! examples/modbus_saul: add example application

f254122

fixup! tests/drivers/modbus: add test application

a613385

fixup! drivers/modbus: add initial support

ef55a8e

fixup! tests/drivers/modbus: add test application

6f6f606

derMihai reviewed Mar 27, 2024

View reviewed changes

drivers/modbus/modbus_rtu.c Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: modbus_rtu: add initial support (v2) #20386

drivers: modbus_rtu: add initial support (v2) #20386

basilfx commented Feb 13, 2024

kfessel Mar 14, 2024

basilfx Mar 19, 2024

basilfx Mar 20, 2024

MrKevinWeiss left a comment

MrKevinWeiss Mar 19, 2024

basilfx Mar 19, 2024

basilfx Mar 20, 2024

riot-ci commented Mar 19, 2024 •

edited

Loading

kfessel Mar 21, 2024

basilfx Mar 25, 2024

kfessel Mar 21, 2024

basilfx Mar 25, 2024

basilfx commented Mar 25, 2024 •

edited

Loading

kfessel Mar 26, 2024 •

edited

Loading

basilfx Mar 26, 2024 •

edited

Loading

kfessel commented Mar 27, 2024

		@@ -0,0 +1,34 @@
		# Copyright (c) 2023-2024 Bas Stottelaar <[email protected]>

	uint32_t rx_timeout; /*< time between two bytes before timeing out /
	uint32_t rx_timeout; /*< timeout between bytes/

drivers: modbus_rtu: add initial support (v2) #20386

Are you sure you want to change the base?

drivers: modbus_rtu: add initial support (v2) #20386

Conversation

basilfx commented Feb 13, 2024

Contribution description

Testing procedure

Issues/PRs references

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrKevinWeiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riot-ci commented Mar 19, 2024 • edited Loading

Murdock results

Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basilfx commented Mar 25, 2024 • edited Loading

kfessel Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

basilfx Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

kfessel commented Mar 27, 2024

riot-ci commented Mar 19, 2024 •

edited

Loading

basilfx commented Mar 25, 2024 •

edited

Loading

kfessel Mar 26, 2024 •

edited

Loading

basilfx Mar 26, 2024 •

edited

Loading