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? |
| 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?
Sure, Is it possible to generate some documentation with UML diagrams? I don't know much about document generator used by ESPEasy. And it should not be part of the user manual... With the callbacks the code is quite complex and if other developers want to build plugins on the Modbus facility they should be aware of the caveats. |
Well it is at least much easier to 'read' when it is rendered into some diagram. |
I am using PlantUML to specify UML diagrams. There is a plugin for VisualCode. I will upload SVG drawings generated by PlantUML. I will try to create some additional documentation in text. |
|
Question: how can I show the connected Modbus link on the device overview page? For I2C the port column shows "I2C "
|
There is a function |
|
I tried to compile for ESP8266 and it failed on existence of ESPEasy_key_value_store. Having the Modbus link configuration stored using KVS and most likely ESP8266 has limited memory I propose to disable the plugin and the Modbus facilities for this CPU. |
|
You deleted close to 2000 lines in docs/source/Plugin/_plugin_sets_overview.repl |
Yes please do (for now) |
Pulled this local and compiled the current state of affairs (custom_274_ESP8266_4M1M and custom_312_ESP8266_4M1M).
You'll have to do the functional checks on ESP8266, as I don't have any Modbus hardware available 👼 |
That file will be re-generated when the PR is merged, and the documentation uploaded to RTD. Git still doesn't understand that we excluded that file via |
Sorry I am not sure If I have to revert or not. Unfortunately I need some help if reverting is required. I am not a git expert and I don't want to ruin this project... |
|
Don't worry about that file, we'll generate & commit a correct one later |
| /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| // Provide a new transaction structure that can be used to build a Modbus request and queue it at this link | ||
| /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// | ||
| Modbus_RequestQueueElement * ModbusLINK_struct::newTransaction(struct ModbusDEVICE_struct *device) |
There was a problem hiding this comment.
This is really tricky as you easily create a memory leak.
Better let this return an std::unique_ptr<Modbus_RequestQueueElement> and when storing into the queue you need to do a takeover.
This way as soon as the unique pointer gets out of scope, it will be destructed.
Another option is to create some queue or list of the objects and then use a constructor of this class which will for sure set the state to something that will not be processed from the queue.
This way you can add a new entry using emplace_back or something depending on the queue type.
After emplacing a new item, you can use something like auto it = _queue.back(); and when this isn't the same to _queue.end(), you can set the values.
This queue can be of type Modbus_RequestQueueElement or an unique_ptr to it, so you can also deal with allocation failures.
See how controller queue is doing this as this is exactly how it is done there.
|
|
||
| case ModbusQueueState::ERROR_OCCURRED: | ||
| { | ||
| it++; |
There was a problem hiding this comment.
The idea here is to inform the initiator of the transaction (the Modbus_device) uses this or the RESPONSE_RECEIVED state to interact with the result and then set the state to READY_FOR_DESTROY. I am not experienced in queue handling patterns in C++, maybe there is a more robust way to manage ownership and destruction. Coming from high performance embedded C I also don't want to spill too much overhead in copying data between the various objects. I will study the controller queue.
|
I am still in doubt if the setup is right for a simple embedded processor. The main idea is to have a queue owned by the Modbus_link The queue is evaluated on a timer base (10 per second). The Modbus_device knows the protocol, encoding/decoding the messages and queuing the transaction to the Modbus_link. Once a transaction is done a callback to the Modbus_device is done. Note that in theory the device could also poll/check the result when it does not fill in the owning device in the transaction. I don't see a valid reason to do so with the latest design. Also note that, as mentioned by TDer in the review above ownership and lifecycle is a bit tricky. The Modbus_device takes the initiative to create a transaction, then it is on the Modbus_links queue, and finally the Modbus_device is responsible to mark it as READY_FOR_DESTROY and the Modbus_link will destroy it. In the Modbus_device the write transactions (and any unrecognized transactions) are just destroyed once the response is received (or timeout occurred). For read transactions I now have 3 options (evolved during the development).
To keep the administration in the Modbus_device and plugin simple all context is associated with the transaction on the queue. So Modbus_device and plugin need to recognize what they have asked from data returned in the callback or in the PLUGIN_TASKTIMER_IN event. This prevents the need for queues in the Modbus_device and plugin and the overhead of matching pending requests to received answers. An issue is that this requires the transaction to be kept until it is fully handled by the Modbus_device. As initially I did not have a callback from the Modbus_link processing I decided to wait for destruction until the Modbus_device marks it as such. If we decide to make the callback subscription mandatory the modbus_link can destroy the transaction immediately after the callback. This decision will block asynchronous handling by the Modbus_device in future. But I doubt if that would be useful. device and link are very closely coupled and I don't see a reason to postpone the processing by the Modbus_device. In the current implementation the Modbus_device owns the transaction for a short while such that it can fill the request. As suggested this could be solved by keeping it in full ownership of the Modbus_link by adding a CREATED state in which the queue will not process it. To be discussed if we want to add logic to assure the sequence is adjusted once it is queued or just keep the sequence according to the creation sequence. Mind that multiple devices might use the same link. I was not sure if the sendEvent() function was a synchronous call to the plugin or queuing it to an event queue. I learned that it is a synchronous call which makes the event just a callback. This might optimize the current code a bit. It does imply that we must handle a plugin call that requires immediate result with care. Using a PLUGIN_TASKTIMER_IN event will result in a recursive call to the plugin. Most likely this will work, but requires a deep stack. Due to the history of the design currently blocking Modbus reads are handled by a separate function on the Modbus_device and the plugin shall call the process() function to assure the reception of the response is handled. I see 3 options here:
|
|
In my experience, it is always much easier to think of code if things are guaranteed to be done. The unique pointer does have its drawback for some specific use cases, as it can only be a single owner of the element it points to and if you would like to hand it over and it needs to remain in existence, you need to do a take-over of the pointer. Anyway, the gist of this is, move the responsibility of proper destruction of the object to the destructor of the object and then wrap it in either some kind of unique/smart pointer type. Or just have a list of elements of these objects. Taking this simplicity a bit further, then we can have a very basic data structure like this: struct MB_handler {
// Checking all queues to see what can be done
bool loop()
{
bool res{};
for (auto it = _queues.begin(); it != _queues.end(); ++it) {
if (it->second.loop()) res = true;
}
return res;
}
// Get the right queue given a task index
MB_Queue* getQueue(TaskIndex taskIndex);
private:
std::map<TaskIndex, MB_Queue> _queues;
};
struct MB_Queue {
// acting on the _command_list.front() element
// When done
bool loop()
{
bool res{};
for (auto it = _command_list.begin(); it != _command_list.end(); ++it) {
if (it->loop() == MB_state::Finished) {
res = true;
it = _command_list.erase(it);
} else {
return res;
}
}
return res;
}
void add(MB_command& cmd);
MB_port_definition _port;
std::list<MB_command> _command_list;
};
struct MB_command {
MB_state _state = MB_state::Uninitialized;
CallBackInfo _cbInfo;
// Do whatever is needed to process the state machine (incl callback stuff) and return current state
MB_state loop();
};This is just some way too short illustration on how it could be processed. |
|
I think the Modbus_link was close to the example, but I obfuscated it due to legacy in the development. The newTransaction() should be replaced by a proper constructor of the queue element to make clear the transaction is initially owned by the Modbus_device. Once queued it is owned by the Modbus_link (placed on the queue) where it stays until destroyed. In the callback to the device the transaction pointer is only passed to retrieve the result. Then I can destroy the transaction after the callback. I only need the READY_FOR_DESTROY state if the Modbus_device cancels all pending transactions, most likely because it will be destroyed itself. This invalidates the callback references pending on the queue! I did not destroy the transactions directly in the freeTransactions() because it is unclear for me if the process trigger (currently ten per second timer) is synchronized with the plugin event handling. Marking it for destruction and handling all destruction in the same process handling safer. If ESPEasy architecture assures only cooperative multi threading this can be dropped. Not sure I have to use the smart pointer here to transfer the transaction once from Modbus_device to Modbus_link when queuing it. I will upload a cleaned version soon. |
Well I guess keeping the entire modbus command object is a bit tricky and probably also larger than what is needed for ESPEasy to process the received reply. Like I said, programming is much simpler if you can safely assume certain things will be dealt with at the appropriate place. The Modbus handler is just about managing the queues. |
3 participants
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.