P183 modbus RTU

[flashmark]

Initial version of a simple Modbus RTU plugin. This plugin handles a Modbus device as a set of holding registers. Up to 4 holding registers can be read into the 4 values of the plugin. The plugin number is 183 as agreed elsewhere.

[TD-er]

Can you look at the timing stats for this plugin you added?
I suspect the reading like this will take 100's of msec per register read.

Please take a look at what I did for the Eastron plugin, where I set a queue of registers to read.
This way the waiting time per call is nihil and thus not blocking the rest of the ESPeasy functionality.

My intention is to make all ESPEasy plugins using modbus use this approach to share access to the same Modbus bus.

Also the ESPEasySerial for (nearly) all plugins is using the same set of PCONFIG() for the serial config.
I did not check if you did this, so please make sure to use the same way as in the most recent plugins using a serial port.

[flashmark]

I have been busy for some time. Try to pickup the comments soon. Indeed the exchange takes some time and I will look into creating a queue. Eastron was my inspiration, but I did not look in most of the details as it was mainly device specific handling. Serial settings were copied from Eastron. To share the modbus I need some more insights how the sharing is intended. I have too little experience with some details in ESPEasy.

[flashmark]

A small introduction of myself. I started as embedded software developer using mainly C, but I am for quite some time software architect and not doing professional coding anymore. I am definitely not a C++ expert. I am working for a large company building complex machines. I like to pickup smaller embedded software projects and domotica. And I really like the way ESPEasy is set up.

I see some issues with the current P078 implementation. The modbus and Eastron device specific features are mixed over the various "layers". There is the "plugin" that takes care of the configuration and external data (variable, config, web representation). Then the "data_struct" to put the behavior of the plugin in a C++ friendly environment (away from .ino). And there is a "Eastron library" in SDM.cpp.

If understood you well the requirements are:

• Multiple plugin instances of different plugin types can share the same physical Modbus. (Use the same link)
• Multiple links should be possible in parallel
• Modbus access shall not hold the CPU during message exchange

The P078 implementation uses a queue that can manage multiple plugins in theory. For me it is unclear how it can differentiate between multiple links. The plugin defines the serial link, if there are multiple plugins connected it seems the last plugin that initializes defines the link properties. Is this desired behavior?

The queue knows it is handling SDM messages and delivers the received holding register directly into the uservar: UserVar.setFloat(it->taskIndex, it->taskVarIndex, value);
It is a fast way to handle the data, but it makes it impossible to retrieve other data from the Modbus device.

My proposal would be to split the code into the following classes:

• Plugin & optional data_struct to handle the user interaction as standard in ESPEasy
• A Modbus class that handles one link. This includes a queue and Modbus packet coding/decoding.
• A singleton Modbus "broker" that manages a list of Modbus links. The broker handles init/terminate requests from the plugins and tries to combine requests from multiple plugins in a single Modbus link.
• Existing ESPEasy serial link class responsible for the data transport over the selected serial link

Broker and link should be outside any plugin as they can be shared by multiple plugin classes. As they are both Modbus specific they can be in a single file sharing a header file.

I still need to think about the details how to exchange data and fit the queue neatly in the design. What has to be in and how does it return results. The Modbus link should be able to handle both the RTU binary and ASCII format. It should be able to handle other Modbus message types. Both option for future only :-)

One thing to consider:
Do we want to have the serial link specified through the plugin or add Modbus links as dedicated resources to the hardware page? My preference would be to keep it as is. Will make the broker a bit more complex, but we can manage that.

[TD-er]

Well hats off to you sir, as you seem to have a very good idea of the layers we have right now :)

Right now, I am working on another pull-request to do a complete rewrite for ESPEasy's WiFi/network implementation.
This does add a "Network" tab, just like the current "Devices" and "Controllers" tab we currently have.
And when I say "just like", it really is very similar.
So the first table is showing all network adapters and a short overview of its current state/config.

When clicking "Edit" on such an adapter, you get the specific settings, very similar to how controllers and tasks are being dealt with.

My next idea for a follow-up on this is to add a tab for "Interfaces" (or buses, name is not yet clear).
This way you can define interfaces like Modbus, SPI, I2C, 1-wire, etc. (maybe also extended GPIO pins)

Especially some of those interfaces which share a bus for multiple devices, need a handler to deal with all devices and pending transactions on the bus.
And also as you rightfully mentioned, some main (singleton) handler to handle all interfaces of the same type.
For example, when requesting a read or write on a modbus device, you must make sure you have completed this before something else is requested.

This idea is already implemented (in a bit too specific way) by keeping track of a list of registers to read and where to put the result.
This does work fine for Eastron devices, as it is all the same. You call for a register and interpret the result, then store the value somewhere.
However this already has some limitation as it only assumes float values. There are however some registers which do not return a float.

So to "fix" this, I imagine there might be some new PLUGIN_MODBUS_READ_RESULT call, which then should be implemented in those plugins which support Modbus communications.
Then in the event (ESPEasy EventStruct), there should be the taskindex set and probably some other values too and a pointer to the read data.
Those plugins then should request a read to the Modbus handler from the PLUGIN_READ call.
This way the read is already way more generic.

Then adding a main handler would probably be something for a next pull-request so we can think of a more generic way to manage modbus handlers.

The main advantage with this is that there is no longer a collision when accessing modbus devices on the same bus and there is no active blocking wait for a reply from the device, which may take quite some time.
For example the SenseAir S8 may take 200+ msec to reply and currently does actively wait.
The same for the ACU28x (or how those power meters are called...) and probably also for those PZEM meters.

[flashmark]

Looks good. By the way, I am not in any hurry to push my branch. Please continue the framework and let me know where I can contribute for something modbus specific. A suggesting is to remove the word MODBUS in the event and keep is a bit more abstract like BUS_READ_RESULT. This can then be any pending bus transaction. I think that I2C could also benefit.

If we add this callback trough an event and a central bus or link administration the singleton management layer will be very simple. The plugin know the bus type and index and the manager returns the associated bus object. Maybe do some admin to check how many active plugins are coupled to the bus; and do initialization termination when the first joins or the last leaves.

By the way, I saw a Modbus_RTU file. Is this where the new bus management stuff should grow?

[TD-er]

Not likely that this will remain the (file) structure.

For the WiFi/network rewrite, I'm introducing namespaces like ESPEasy/net/wifi

The idea of having a generic bus callback/event also seems OK, as long as the bus manager/handler does keep in mind which task may be expecting specific bus responses.
Like there are certain devices which can be used via various different bus types. For example the same sensor on I2C or SPI and/or UART.
But that's something to worry about later.

[flashmark]

Sorry it is still work in progress I wanted to set aside. I had some other duties and am just picking up this project. Will update it soon with a more crisp design. Keep in mind: I am not a C++ coder, any corrections and suggestions are welcomed.
Main design separation into several classes:

• Plugin: Unique for a device and implementing the interaction with ESPEasy.
• Modbus_device: Represents one modbus device and is coupled to a plugin. Does the modbus message (de)coding.
• Modbus_link: Represents a physical modbus link (serial link). One link can serve multiple modbus_devices.
• Modbus_mgr: Connects the devices and the links. This singleton object knows the devices and the links. Main task is to create a link when the first device wants to connect and connect a device to a link on request.
• Queue: Each link has a queue of pending modbus transactions.

Main issues to resolve:

• Serial port configuration is connected to a modbus_link. But I can only configure a plugin. Workaround: last device connecting to a link determines the properties. (If the ESPEasySerialPort type is different then it is a new link, still to think how to handle multiple "software serial")
• Transaction takes too much time to wait/poll. Instead we use a queue to prepare transactions (one message exchange). As a result we need some callback mechanism to return the results.
• For the device to link relation the is a real callback is implemented as a class function and the link knows the modbus_device that queued the transaction.
• For the plugin to device relation I don't know how to properly implement a mechanism that can feed back the uint16 registers from the modbus reply. Converting them to plugin output values blurs the responsibilities of the plugin and the modbus_device. This will make reuse much more difficult (like the P078 plugin).
• To keep the link busy it needs to poll the serial link for a complete reply. For now this is done by a ten_per_second trigger from each plugin. This is a bit heavy on CPU load. I am looking for a direct triggering of the link objects. The code should be prepared for that.

Is there a way to store design documentation with a plugin?

[TD-er]

Why not use some (weak) std::shared_ptr like structure for this?

[TD-er]

I did add a few comments.
Did not yet finish all of the code, just the things that immediately came to mind.

[flashmark]

Is there a convenience function to select a serial port port type from a drop-down list? The caveat is that the list depends on the supported hardware. I now create a mapping from index to ESPEasySerialPort that only contains the valid values.

[TD-er]

Is there a convenience function to select a serial port port type from a drop-down list? The caveat is that the list depends on the supported hardware. I now create a mapping from index to ESPEasySerialPort that only contains the valid values.

You can take a look at serialHelper_webformLoad
But I'm not sure that function is already usable for multiple selectors on the same page.

Also I think we may need later to include some other flags to excluse ports like USBCDC and HWCDC.

[TD-er]

In the end the Modbus config will have to move to the Interfaces tab.
So there probably needs to be a helper function anyway to allow for multiple serial config selectors on the same page/tab.

[flashmark]

In the end the Modbus config will have to move to the Interfaces tab. So there probably needs to be a helper function anyway to allow for multiple serial config selectors on the same page/tab.

I already added it to the interfaces page. However, I decided to put the code to present/read the data in the Modbus_mgr. This follows the same pattern as for the plugins. Otherwise the interfaces code will become very big. It will also create many dependencies on all kind of conditions. Keep local problems local.

[flashmark]

I keep the current conversion code for the Serial port selection. Let's see if we can come to a generic helper function later. Main issue is indeed which ports to support. For Modbus I assume that only hardware serial and low speed software serial will really work. For RS485 the transducer must be switched after the packet is sent. I now just rely on setRS485Mode().

I changed the baudrate selection as it is not always doubled. There is a discontinuity in the frequent used baudrates.

Some other oops were found during testing. It is pretty difficult to change concepts and assume most of the code can be left untouched.

Will be unavailable coming weekend. Picking up later.

[flashmark]

How can I see how much time is spent in the 10-per-second processing of the Modbus_link? I just hooked it somewhere in the timer code. Hook needs some reviewing to see if this is compliant with the ESPEasy design. Timing might be interesting to evaluate.

I now have two links in parallel with one link on 4800 baud with one device (my original rain sensor) and one link on 9600 baud with two simple devices as load (temp/hum sensor and relay board).
Getting near to working software ...

Another review is needed on how the Modbus link setup is hooked into the interfaces page. I have chosen to delegate the code to the Modbus_mgr like done for the setup of plugins. But still the interfaces code needs to know the details of Modbus. This might be abstracted by making it a plugin structure and all interfaces implementing the same interface (e.g. events function). But for now it works,,,

[TD-er]

How can I see how much time is spent in the 10-per-second processing of the Modbus_link? I just hooked it somewhere in the timer code. Hook needs some reviewing to see if this is compliant with the ESPEasy design. Timing might be interesting to evaluate.

You can enable the timing stats on the tools->Advanced page.
Then you can let it run for a while and visit the tools->Timing Statistics page.

N.B. every time you visit that page, the stats will be reset, so you might want to visit it once after boot and/or saving settings as those typically mess up the stats quite a bit.

For every task, the typical PLUGIN_xxx ones will be listed when they were called at least once since the last reset of the stats.

Every line with a timing duration ever exceeding 100 msec will be highlighted.

[TD-er]

Another review is needed on how the Modbus link setup is hooked into the interfaces page. I have chosen to delegate the code to the Modbus_mgr like done for the setup of plugins. But still the interfaces code needs to know the details of Modbus. This might be abstracted by making it a plugin structure and all interfaces implementing the same interface (e.g. events function). But for now it works,,,

I will have a look.

[TD-er]

Depending on where/how it is used, you could also decide to return the resultState, and maybe think of a useful enum value for "Success" or failure.
For example the Espressif SDK typically returns ESP_OK, which is a 0 value and all other return values are > 0.

Right now it is only used on a reset() call and when processing the queue.
But in that last one it feels a bit 'odd' because it also returns 'false' when the state is still busy and right at that point you are processing whether the link is occupied.
However there is no distinction between calles with a nullptr or just busy.
On the other hand in this specific case the pointer is not likely to be a nullptr:

            responseOK = it->_device->linkCallback(&(*it)); // Notify the device that a response was received

This to be honest is quite odd, as it would make much more sense to have a linkCallback function in Modbus_Transaction, which just does the check for _device != nullptr and then call the linkCallback with itself.

TL;DR
This is a bit odd, mainly because of how it is called.

[flashmark]

I agree this was a quick fix when decided to wait a recovery period when a transaction fails after a timeout of the previous transaction.

Keep in mind the linkCallback() is only to be called from the Modbus_link to report the result of a transaction.
Modbus_device --> Modbus_link : Queue transaction [context]
... processing by the Modbus_link, might be other transactions from same or other devices...
Modbus_device <-- Modbus_link: linkCallback()
Transaction destroyed by Modbus_link

I needed a return value to determine if the message was OK. By design all decoding is left to the Modbus_device. The Modbus_link cannot see if there was a collision. Therefore I added a return value for "message was decoded OK". For now the Modbus_link does not need more detail.

The nullptr check is there for safety only. The callback is only done with a valid Transaction from a Modbus_link processing that same transaction. A dedicated return value could be added to mark this situation. But I wonder what special treatment the Modbus_link should give when this is returned. Keep in mind that Modbus_device and Modbus_link are intimate friends using the Transaction to pass information. Nobody else should ever see the Transaction or be aware of how tasks are divided between each other.

Started as C programmer and now doing architecture in UML and Powerpoint/Word I still tend to divide between active and passive objects. Where passive objects are just data owned by active objects or passed between them. This resulted in making the Transaction "passive" and only shared between Modbus_device and Modbus_link. It is now fully owned by Modbus_link.

It is indeed possible to delegate some knowledge and actions to the Transaction and make it part of the callback sequence. It may shift the knowledge about the Modbus_device class from Modbus_link towards Transaction. It is also adding another call to the stack in a heavy call sequence. Note that Modbus_device will forward the reception to the plugin using an event. I don't see much value given Modbus_device, Modbus_link and Transaction are intimate and plugin clients only see the Modbus_device. At the other side Modbus_link is only seen from Modbus_mgr to setup the link and its properties. My proposal is to keep it as is or only define a dedicated enum as return value for the callback.

What is a nice philosophical question is how the transaction is initiated. We want the transaction always owned by the Modbus_link, or even be part of the queue owned by the Modbus_link. The transaction is initiated by the Modbus_device> The device produces the content of the quest message and later on interprets the response message. The current design is to let the Modbus_device create a Transaction that is on the queue, but NOT_QUEUED. The returned Transaction can be filled with the proper data and then queued. The nice thing is that the Modbus_device can directly construct the message and fill in the additional data. The ugly part is that we need to handle NOT_QUEUED and we export the Transaction to our friend Modbus_device. A "clean" design should define a Transaction_input and Transaction_result structure. The Modbus_device calls the queue() function with the input structure. The Modbus_link creates a Transaction and copies the the data from the input. Before doing the callback the result is copied from the internal Transaction. Clean, but two additional copy actions for each Modbus transaction. Looking at previous Modbus implementations the current design is already much cleaner. Splitting the functional interpretation (plugin), Modbus message construction & interpretation (Modbus_device) and handling of the queue for multiple devices (Modbus_link). This comes with an overhead cost, I suggest to keep the current, a bit more efficient, design.

[TD-er]

This feels like it should be a member function of ModbusTransaction as it uses all parameters from that object.

[flashmark]

It processes indeed the transaction. But it also needs the context like which serial link is involved. In this specific part it needs to know the result of the previous Transaction.

As written above I wanted the Transaction to be a complete passive intermediator between device and link. The link is the active element going over the queue and picking the Transaction to be put on the wire. If we shift the responsibility to handle part of the transaction handling to this class we also have to make it aware of some context and when to do certain evaluations (coupling it to the 10-per-second handling).

If this remark is about evaluating if the timeout has expired: Yes we can delegate this to the Transaction. A new function it->timeoutPassed() can be introduced. Is used only in the Modbus_link. I expect 2 or 3 times. But it does not make it more efficient not removes the need for Modbus_link to know many details stored in Transaction.

[flashmark]

How can I see how much time is spent in the 10-per-second processing of the Modbus_link? I just hooked it somewhere in the timer code. Hook needs some reviewing to see if this is compliant with the ESPEasy design. Timing might be interesting to evaluate.

You can enable the timing stats on the tools->Advanced page. Then you can let it run for a while and visit the tools->Timing Statistics page.

N.B. every time you visit that page, the stats will be reset, so you might want to visit it once after boot and/or saving settings as those typically mess up the stats quite a bit.

For every task, the typical PLUGIN_xxx ones will be listed when they were called at least once since the last reset of the stats.

Every line with a timing duration ever exceeding 100 msec will be highlighted.
I know this page. But Modbus_link processing is done on "interrupt" base: It is executed 10 times per second to be able to monitor when a complete serial response message is received. T make it a bit more complex and faster to response it is also executed each time a Modbus_device queues a transaction. Therefore the processQueue() shall be robust for the context it is executed in. But the question rephrased: How can I make sure the 10-per-second evaluation is added to this page?

[TD-er]

I know this page. But Modbus_link processing is done on "interrupt" base: It is executed 10 times per second to be able to monitor when a complete serial response message is received. T make it a bit more complex and faster to response it is also executed each time a Modbus_device queues a transaction. Therefore the processQueue() shall be robust for the context it is executed in. But the question rephrased: How can I make sure the 10-per-second evaluation is added to this page?

You can add some enum and all other related functions like for turning it into a flash string.
Then there are macros for start and end of what you like to include in the timing stats.
All the rest is done for you.

Just some random example of how it can be used:

ESPEasy/src/src/Helpers/PeriodicalActions.cpp

Lines 80 to 82 in 2776ae9

	START_TIMER;
	ESPEasy::net::NWPluginCall(NWPlugin::Function::NWPLUGIN_FIFTY_PER_SECOND, 0, dummy);
	STOP_TIMER(NWPLUGIN_CALL_50PS);

[flashmark]

I refactored the link processing in Modbus_link. It should better fit the discussions above where we discovered some issues due to the sequence Modbus_devices may change a transaction from NOT_QUEUED or QUEUED. Now an _activeTransaction is kept with the Modbus_link. This transaction must be completed before a new transaction can be picked up from the queue.

If no active transaction is on the wire the queue is not only checked for the next transaction, but it is also purged from discarded transactions. Note that devices may discard transactions on their own. Typically done when the Modbus_device is destructed.

I will pick up comments from Ton a later moment. Missed them before committing.

[tonhuisman]

This array will be too small to hold all available ports, as MAX_SERIAL_TYPE isn't based on the highest value for a serial port definition.
We'll probably have to add a selector that doesn't store the GPIO pins in the standard Pin Settings fields, like serialHelper_webformLoad() currently does. (I'll see what I can do to solve this)
In that function you can also see how the current selector is filled with all available serial ports for the chip at hand (I've been testing on an ESP32-P4 that has 5 available serial ports, and was missing a couple 😅)

As a workaround I edited lib/ESPEasySerial/ESPEasySerialPort.h to move the serial3..serial5 definitions between usb_cdc_0 and MAX_SERIAL_TYPE.

[TD-er]

As a workaround I edited lib/ESPEasySerial/ESPEasySerialPort.h to move the serial3..serial5 definitions between usb_cdc_0 and MAX_SERIAL_TYPE.

Make sure the enum values themselves (apart from MAX_SERIAL_TYPE ) are not changed as those are stored in the settings.

[flashmark]

I am struggling with the Serial concepts in ESPEasy. For the on-chip hardware ports we have an entry for each one in this ENUM. This is also causing the issues when a new chip introduces new ports or port types. For the software serial connections we can instantiate multiple serial links of this same type. I assume the same holds for the I2C serial. It makes sense to select specific ports if they have hard-code properties like the TX and RX pins. It also makes sense to allow multiple instances of the same type like software serial and I2C serial. But for those we need other means to separate the instances... What is the design behind the serial configuration?

This section of the Modbus link configuration needs some refactoring anyway. What I need is a way to select what type (or instance) of serial link to be used. And according to the selection different parameters may be needed.

For now I just want to make a drop-down selection menu containing the available serial port options in the ENUM. For this I construct a lookup table from dorp-down index to ESPEasySerialPort value. As such the total number of values in the enum should be sufficient. Problem is to filter the non-existing values for the specific platform. Next challenge is to filter the options that are useless for Modus. A challenge touched earlier in this pull-request and not solved yet.

Once we can select the right set of available serial types the parameters to setup that link shall be adapted to the selected serial type. I did not give it much attention as for now my focus was on the functional design and implementation. Getting the link working and improving it with help of TD-er Focus should shift to the configuration of the Modbus links (and documentation). I need a lot of help here as I have no experience with ESPEasy serial communication and the way it is/can be configured.

First step is to determine how to create a setup form that allows configuring multiple serial ports. First field should be the type from a drop-down list showing the available options for that specific platform/chip. As I see it this should be a reusable function provided by the serial subsystem (most likely Plugin_helper_serial or ESPEasySerial). What do we have available or should be added?

[tonhuisman]

First step is to determine how to create a setup form that allows configuring multiple serial ports. First field should be the type from a drop-down list showing the available options for that specific platform/chip. As I see it this should be a reusable function provided by the serial subsystem (most likely Plugin_helper_serial or ESPEasySerial). What do we have available or should be added?

I have started working on this, based on the comments I made above.
It is mostly some refactoring of the code that's currently hiding in serialHelper_webformLoad().

[flashmark]

Thanks @tonhuisman I will wait for results and/or recommendations.

I assume serialHelper_webformLoad() and serialHelper_webformSave() are pretty close. Needs to add an index to allow multiple serial ports on a single page. Unfortunately I don't know anything about setting up web pages with scripts. Maybe also allow a selection of which types of ports to exclude (hardware; software; USB; I2C)