Conversation
|
Can you look at the timing stats for this plugin you added? Please take a look at what I did for the Eastron plugin, where I set a queue of registers to read. 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 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. |
|
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:
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: My proposal would be to split the code into the following classes:
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: |
|
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. 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). 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. 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. So to "fix" this, I imagine there might be some new 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. |
|
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? |
|
Not likely that this will remain the (file) structure. For the WiFi/network rewrite, I'm introducing namespaces like 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. |
|
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 issues to resolve:
Is there a way to store design documentation with a plugin? |
| if (_modbus_link != nullptr) { | ||
| _modbus_link->freeTransactions(this); | ||
| ModbusMGR_singleton.disconnect(_deviceID); | ||
| _modbus_link = nullptr; |
There was a problem hiding this comment.
Why not use some (weak) std::shared_ptr like structure for this?
|
@TD-er Thanks a lot for the advises. I will look into the network code for examples. For info: DE/RE is a single pin. |
As far as I know, nobody is, we all have to do other stuff too to pay the bills. |
|
I have been looking at the kvs storage mechanism. Looks good, but I am not sure about some settings. I can use store() and load() to save to/load from disk. What is the meaning of the parameters?
Question is how to reserve the correct space in the config file for optional modbus links. And how can I store the (max) number of links? |
Yep, we have similar values for "TaskIndex", "ControllerIndex", "NetworkIndex", "NotifierIndex" As you will probably come across it, I better start explaining it right away or else you may get lost in the terminology... So a TaskIndex can be used to determine which PluginID is being used. Now to make it a bit more complex, we also have a DeviceIndex.... For Controllers we have something similar.
For network interfaces we have:
I think it is a good idea to also think of proper names for these concepts for Modbus, as there are several variants of Modbus interfaces like ModbusRTU and Modbus over IP and maybe even more. For network I use 'NW" as prefix. I guess for Modbus you can use "ModB" (as there also exists "mBus" in a wired and wireless variant, which is completely different from Modbus) While writing this down, I was thinking about whether we should have made some "Interfaces" tab for all interface types and then have like 8 or more lines to define various interfaces like SPI, I2C, Modbus, etc. But I think those are too different so really hard to make an universal overview table for them and also hard to make a fitting software interface like what we now do in the various *Plugin.cpp files. I think there should be a table like for the Controllers, Networks, Devices, Notifications where you show all configured Modbus interfaces.
I think it is best to have a look at the NetworkPage how this is done using KeyValue types outside the KV-store. |
|
Some extras....
Yep, that's to check whether it makes sense to load the data. I hope this does help to clear up a bit of how the code is structured and at least what the intent was when writing it :) |
|
I was just brainstorming with Ton and we think a modbus interface can likely be stored in (much) less than 128 bytes, so you could store upto 8 interfaces in 1k. |
|
Thanks, still reading and interpreting the nice response. I agree that the required size is very limited. For now I use 5 integers and a boolean per Modbus link. I fully agree with keeping the max number of links low, 4 seems well enough. The current implementation allows to scale this easily in case of special needs. I am not a Modbus expert. As far as I know there are differences in the protocol when doing Modus over ethernet, There is also a version with different encoding we could add that later too. For that we only need to add a simple protocol selection (enum or even boolean). Let's see if I can find some time to play around. |
|
I think the following is needed:
Since the KV-store does have a bit of overhead, it is perfectly fine to combine some like the bits of the serial specific config into a single byte. You can also chose to not store a value if it is already set to the default. (I think this is part of the KV-store already) |
| #if FEATURE_STORE_NETWORK_INTERFACE_SETTINGS | ||
| case Enum::NetworkInterfaceSettings_Type: | ||
| #endif | ||
| case Enum::ModbusInterfaceSettings_Type: |
There was a problem hiding this comment.
Looks like an #if FEATURE_MODBUS_FAC / #endif is missing here
There was a problem hiding this comment.
Correct. I am struggling a bit when to use conditional code or just keep the code clean with a small bit of overhead. But in this case the enum definition is also conditional and thus compilation will break without a guard.
|
|
||
| case PLUGIN_WEBFORM_LOAD: | ||
| { | ||
| addFormNumericBox(F("Modbus Link"), P183_LINK_ID_LABEL, P183_LINK_ID, 0, 3); |
There was a problem hiding this comment.
That magic number 3 should be based on the #define set elsewhere
| # endif // ifndef DAT_OFFSET_CUSTOM_CONTROLLER | ||
| # endif | ||
| # ifndef DAT_MODBUS_INTERFACE_OFFSET | ||
| # define DAT_MODBUS_INTERFACE_OFFSET 6144 // each Modbus link config = 256 bytes, 4 max |
There was a problem hiding this comment.
This is different for ESP8266 and ESP32.
There was a problem hiding this comment.
I wonder if we should enable the Modbus link on ESP8266 at all due to the strict size limitations. But that is a different discussion. Where should this land for the ESP8266? Where can we find the layout in a human readable/understandable format? I already fort how I came to this magic number
There was a problem hiding this comment.
The 'basic settings' are stored from the start, so you can base it on the DAT_BASIC_SETTINGS_SIZE
However for ESP8266 we do have some FEATURE_NON_STANDARD_24_TASKS (not for official builds, only for some custom builds), so I would suggest to take that into consideration for the defines.
Something like this:
# ifndef DAT_MODBUS_INTERFACE_OFFSET
# ifdef FEATURE_NON_STANDARD_24_TASKS
# if defined(FEATURE_MODBUS_FAC) && FEATURE_MODBUS_FAC
# error "Not yet defined where to store modbus data, see https://github.com/letscontrolit/ESPEasy/pull/5390#pullrequestreview-4124760094 "
# endif
# else
# define DAT_MODBUS_INTERFACE_OFFSET DAT_BASIC_SETTINGS_SIZE // Stored in the 1k right after the basic settings, each Modbus link config = 256 bytes, 4 max
# endif
# endif| _easySerial->read(); | ||
| } | ||
| } | ||
| rs485Mode = _easySerial->setRS485Mode(dere_pin, collision_detect); |
There was a problem hiding this comment.
First assignment to this variable is never used, so that can be discarded, and this can be changed to
const bool rs485Mode = _easySerial->setRS485Mode(dere_pin, collision_detect);for better compile-time optimization
| if (_easySerial == nullptr) { | ||
| addLogMove(LOG_LEVEL_INFO, F("Modbus: Link, Serial port not initialized")); | ||
| return; // Serial port not initialized | ||
| if ((!_initialized) || (_easySerial == nullptr)) { |
There was a problem hiding this comment.
This can be a class method: (in the .h file)
bool isInitialized() const { return _initialized && (_easySerial != nullptr); }so you can use if (!isInitialized()) { return; }
| int16_t baudrate, | ||
| int8_t dere_pin, | ||
| bool collision_detect = false); | ||
| bool isInitialized() const; |
| ESPEasySerialPort ModbusLINK_struct::getPort(void) const | ||
| { | ||
| return ESPEasySerialPort(); | ||
| return ESPEasySerialPort(); |
There was a problem hiding this comment.
Not sure if this is the intended or desired return value...?
| int16_t ModbusLINK_struct::getSerialRX(void) const | ||
| { | ||
| return _easySerial != nullptr ? _easySerial->getRxPin() : -1; | ||
| return _easySerial != nullptr ? _easySerial->getRxPin() : -1; |
There was a problem hiding this comment.
Should isInitialized() be used here instead? Same for the other return values
| linkInfoPtr = _modbus_links[i]; | ||
|
|
||
| ///////////// linkInfoPtr->link->processCommand(); // Trigger processing of the command queue on the link | ||
| if (_initialized) { |
| @@ -152,7 +145,7 @@ uint16_t P183_data_struct::readRegisterWait(uint16_t address) { | |||
|
|
|||
| while (state == ModbusResultState::Busy) { | |||
| delay(50); | |||
There was a problem hiding this comment.
Hmm, delays should be avoided like the plague, processCommands() should be called in 50/sec or 10/sec events instead



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.