-
Notifications
You must be signed in to change notification settings - Fork 2.2k
P183 modbus RTU #5390
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: mega
Are you sure you want to change the base?
P183 modbus RTU #5390
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? |
src/src/Helpers/Modbus_device.cpp
Outdated
| if (_modbus_link != nullptr) { | ||
| _modbus_link->freeTransactions(this); | ||
| ModbusMGR_singleton.disconnect(_deviceID); | ||
| _modbus_link = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use some (weak) std::shared_ptr like structure for this?
|
Question: Is there a place to store design documentation? I want to make some UML drawings to clarify the rather complex design with several classes involved. |
|
Good question. On one hand I would like to have it close to the source code related to it. So I'm not sure yet and we don't want to loose this kind of info. |
|
I will be on holiday end of this week and away for 6 weeks. I will pick up this pull request once being back. I assume the code still needs some refinement. |
|
I am back from holiday and added some documentation in code and misc/modbusFacility. The plugin is still experimental, but should be shareable. For better support of queued actions ESPEasy should provide a standard mechanism to report updated values to a plugin or plugin_struct. This removes a lot of indirect polling at ten_per_second. See second sequence diagram. Another improvement would be the possibility to specify a Modbus link as separate object in the web GUI. This should be similar to the I2C bus. With such an addition the serial port parameters can be removed from the plugin and replaced by a simple reference to the Modbus link index. The Modbus_device functionality is limited to read & write holding registers. Extensions may be needed for some devices/plugins. I expect this will fit pretty well in the current design. Currently I am not available to implement such changes. I started this plugin to play around with a very simple Modbus device (rain sensor) which is now working in a custom build. My proposal is to integrate the plugin as "experimental" and not add it to any collection. This allows the tweakers to play with it without the risk of incompatible changes in the future. I am available for future support for the plugin. |
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.