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]

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)

[tonhuisman]

Hmm, as said earlier, I've already started work on handling this situation. This will make merging stuff much harder 🤔

[flashmark]

We can leave out my part, as long as I can get the serial type and ports returned... I just hacked the original code such that each port has its own script by adding a sequence number.

[flashmark]

I tried to generalize the plugin_helper_serial library. Two functions added that allows configuring multiple serial ports on a web form. It mimics the set used to configure the port for a plugin. Note that I have no experience on web forms or scripts. This needs a good review. Once accepted we can look in code de-duplication with the original functions.

I also saw that the flag allowSoftwareSerial is not used in the code. But I don't see an easy way to remove an entry from a static array definition.

[TD-er]

But I don't see an easy way to remove an entry from a static array definition.

Another idea can be to add an optional array of bool (or other type for attributes) like we have on some similar other functions to add a selector.

[tonhuisman]

But I don't see an easy way to remove an entry from a static array definition.

Another idea can be to add an optional array of bool (or other type for attributes) like we have on some similar other functions to add a selector.

That is taken care of in my refactored code 😉

[flashmark]

Another idea can be to add an optional array of bool (or other type for attributes) like we have on some similar other functions to add a selector.

That is taken care of in my refactored code 😉

Keep me posted when the code is available for testing. If it allows to show multiple serial ports on a single page with a show and read back function per port it should be easy to integrate in the interfaces-Modus page I created.

[tonhuisman]

Keep me posted when the code is available for testing. If it allows to show multiple serial ports on a single page with a show and read back function per port it should be easy to integrate in the interfaces-Modus page I created.

Well, you'll have to revert your changes to _Plugin_serial_helper.cpp and .h to be able to merge my refactored code. I've also applied optimizations to the javascript code, to allow for the hiding/showing of the GPIO and I2C Serial parameters, when those are enabled.
And to test all my changes, I already adapted Modbus_mgr.cpp and .h, to correctly align with the refactored selector, and did some other improvements too. But that's a drop-in replacement for your files.

I expect my PR to be merged soon, so you should be able to update with latest mega when that's happened. (There's another feature, no PR yet, that's waiting for that refactored code 😅)

[flashmark]

I expect my PR to be merged soon, so you should be able to update with latest mega when that's happened. (There's another feature, no PR yet, that's waiting for that refactored code 😅)
Just saw the message. Thanks for the new functionality. I will try to pick it up asap once I rebased to your changes.

[flashmark]

I am not very experienced with github. I try to sync with the mega branch, but that results in a warning to drop all my commits to get in sync again. How can I do a manual merge? I would like to drop all changes in the serial library and manually fix other issues.

Where can I find the changes made by tonhuisman? Are these already in mega?

[tonhuisman]

Are these already in mega?

My changes have been merged into mega, that's probably causing these merge conflicts... 🤔

[flashmark]

Simple fix if you know how to get to the conflicts :-)
There was a single merge conflict only...

[tonhuisman]

Here's a .diff file (zipped so GH will allow the upload). The extracted file should be placed in the root of your local repository, then from a terminal (best to open/use that from inside VSCode, shortcut: <ctrl>-<shift>-<backtick>) use the command:
git apply P183-Modbus_mgr-tonhuisman.diff

That should go without any issues.
Besides the updated configuration stuff, I made a couple of optimizations, mostly replacing the rather expensive strformat() calls with concat(), where you're only appending a number to a string.

P183-Modbus_mgr-tonhuisman.zip

[flashmark]

Here's a .diff file (zipped so GH will allow the upload). The extracted file should be placed in the root of your local repository, then from a terminal (best to open/use that from inside VSCode, shortcut: <ctrl>-<shift>-<backtick>) use the command: git apply P183-Modbus_mgr-tonhuisman.diff

That should go without any issues. Besides the updated configuration stuff, I made a couple of optimizations, mostly replacing the rather expensive strformat() calls with concat(), where you're only appending a number to a string.

P183-Modbus_mgr-tonhuisman.zip

It states "error: No valid patches in input (allow with "--allow-empty")". And when double clicking the diff it states "Already up to date". But I don't see modifications in mudbus_mgr.cpp showing how to use the new library.

I am missing a clue on how I can show multiple, separate, serial links on the same web page as I do for the Modbus interface(s). I would like to see your modified version.

[tonhuisman]

Hmm, not sure what went wrong, here's an updated diff:
P183-Modbus_mgr-tonhuisman-2.zip
I added a -2 to the .diff filename 😉

[flashmark]

Hmm, not sure what went wrong, here's an updated diff: P183-Modbus_mgr-tonhuisman-2.zip I added a -2 to the .diff filename 😉

Thanks a lot, working on it.