Skip to content

P183 modbus RTU#5390

Open
flashmark wants to merge 44 commits intoletscontrolit:megafrom
flashmark:P183_Modbus_registers
Open

P183 modbus RTU#5390
flashmark wants to merge 44 commits intoletscontrolit:megafrom
flashmark:P183_Modbus_registers

Conversation

@flashmark
Copy link
Copy Markdown
Contributor

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.

Comment thread src/_P183_modbus.ino
@TD-er
Copy link
Copy Markdown
Member

TD-er commented Aug 31, 2025

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

TD-er commented Sep 6, 2025

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Member

TD-er commented Sep 6, 2025

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.

Comment thread src/_P183_modbus.ino Outdated
Comment thread src/src/Helpers/Modbus_mgr.cpp Outdated
Comment thread src/src/Helpers/Modbus_link.h Outdated
Comment thread src/src/Helpers/Modbus_link.cpp Outdated
Comment thread src/src/Helpers/Modbus_link.cpp Outdated
Comment thread src/src/Helpers/Modbus_link.cpp Outdated
Comment thread src/src/Helpers/Modbus_link.cpp Outdated
@flashmark
Copy link
Copy Markdown
Contributor Author

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?

Comment thread src/_P183_modbus.ino Outdated
Comment thread src/_P183_modbus.ino Outdated
Comment thread src/src/Helpers/Modbus_device.cpp Outdated
if (_modbus_link != nullptr) {
_modbus_link->freeTransactions(this);
ModbusMGR_singleton.disconnect(_deviceID);
_modbus_link = nullptr;
Copy link
Copy Markdown
Member

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?

Comment thread src/src/Helpers/Modbus_device.cpp Outdated
Comment thread src/src/Helpers/Modbus_device.cpp Outdated
Comment thread src/src/Helpers/Modbus_device.cpp Outdated
@flashmark
Copy link
Copy Markdown
Contributor Author

@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.
I will also look at the other comments and fix them. I assume some new reviews are required once all is in place :-).
First to assure storage, next to remove configuration in the plugins/device and last add callback to plugins.
As you can see I am not full time programming for ESPEasy, please be patient...

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 2, 2026

As you can see I am not full time programming for ESPEasy, please be patient...

As far as I know, nobody is, we all have to do other stuff too to pay the bills.

@flashmark
Copy link
Copy Markdown
Contributor Author

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?

  • SettingType::Enum seems to indicate what type of data. What type to use for new Modbus related parameters?
  • the index seems to be used to index an array of the same type of data. I assume this can be used to identify individual Modbus links. Each link has its own kvs structure with another index assigned.
  • offset_in_block is unclear and as far as I saw in the examples is set to zero.
  • id_to_match looks like a "magic number" to check if the data in the file really matches the intended module. The network plugin identifier seems to be used for network plugins.

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?

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 8, 2026

First of all, you have to have some kind of idea on how much data you may need.
If you call meminfodetail in the command line field on the Tools page, and then open the sysinfo page (too bad we're already past Easter... ;) ) you will see at the bottom of the page how the settings file is structured. (N.B. this structure is different for ESP8266 and ESP32. This feature may be left out on builds with LIMIT_BUILD_SIZE defined)

Here the example for ESP8266 as that's shorter:
image

Here you can see that for example each task does have 2 blocks of 1k per task (red for TaskSettings and green for CustomTaskSettings)
Then there are 1k blocks of ControllerSettings and 1k blocks of CustomControllerSettings.
The top line shows the overview of the entire settings file.

As you can see, there is a gap of unused data between the reserved space for the SettingsStruct and the first task and there is a gap between the 3rd ControllerSettings and the first CustomControllerSettings block.

In the ESPEasy code, there are per settings type some hard coded values like offset in the config.dat (or one of the other files), size of the block and how many blocks there can be.

The offset you mentioned is likely the offset in such a block.

The KeyValueStore does consider an allocated block in a settings file as its own 'playground'. When writing a set of key/value tuples, the index is stored along with the data type and this then determines how to interpret the data.
For example a bool is stored as either a 'true type' or a 'false type', which then needs 0 extra bytes to store the value. :)
Then there are various 1 byte types, 2-byte types, 4-byte, etc.
So a KV-store should be read at once and written as once and you should not have to deal with the specifics of the internals. This is just FYI, to have some idea how it is working and what to expect.
Before the KV-store, we used to store C-structs as a binary blob in the settings. This is working fine, until you need to change the type of some value. Then we created an array of Strings, which has the benefit of not requiring to waste a lot of space and allow for variable length values (ideal for display texts for example) as long as you didn't use more than the typical 1k of reserved space.
Then Ton came up with the idea of starting in the CustomTaskSettings and if there was a need for more space, continue in a separate file specific for that task. N.B. this also results in an incorrect display of this settings layout chart as the size is a virtual 5k for CustomTaskSettings on those (ESP32-max) builds supporting this feature. (still need to fix this chart to represent this correctly)

Check the ESP32 chart to see what I mean with the '5k bug' in the CustomTaskSettings:
image

N.B. there is also a 1k unused block between the (6k) SettingsStruct and CDN_url block starting at 7k offset:
image

So back to the idea of how much data is needed for defining a modbus interface.
I think 256 bytes is more than enough and it is also unlikely to have more than 4 modbus interfaces on a single node as there are not many ESPs with even 4 HardwareSerial ports. Of course you can add more by using software serial or some I2C to UART chips we support. But I guess when you need so many modbus interfaces you either have not a clear understanding of modbus, or you could chose for more ESPEasy nodes as Modbus typically can also used over quite long bus lengths.

So then you could fit those 4 interfaces easily into a 1k block and I suggest you'll place those right after the SettingsStruct area. So on ESP8266 starting at 3k offset and on ESP32 at 6k offset.

Then you will use a block size of 256 bytes and the "ModbusInterfaceIndex" of 0 ... 3 will determine where the reserved blocks will be.

So there also needs to be a new SettingType::Enum type for the ModbusInterface and I think that will probably be a great name :)

OK, will post the rest in a new reply.

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 8, 2026

the index seems to be used to index an array of the same type of data. I assume this can be used to identify individual Modbus links. Each link has its own kvs structure with another index assigned.

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...
Take for example the concepts for Plugins and Tasks.
We have a Plugin, which refers to a piece of code to interact with some hardware. For example P004 for the 1-wire temperature sensors with PluginID 4.
Then there is a Task, which is assigned a plugin-ID. For nearly all plugins we now support multiple task instances to use the same plugin ID. For example multiple tasks to operate multiple DS18b20 temperature sensors.

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....
As you know we do have several builds, like the 'normal', 'max', 'climate', etc.
These all have different sets of plugins included in a build.
For example you have a build with P004 (1-wire temp sensor), the Dummy plugin (P033) and the OLED Framed plugin (P036).
So we have a Device array of 3 elements referring to PluginID 4, 33, 36.
For this, we have functions to do a lookup between TaskIndex, PluginID and DeviceIndex.

For Controllers we have something similar.

  • ControllerIndex, similar to TaskIndex (thus the row number in the tables in the ESPEasy UI, only starting at 0 where the UI starts at 1)
  • CPluginID similar to PluginID (for example C005 has CPluginID 5 and is the OpenHAB/HomeAssistant MQTT controller)
  • ProtocolIndex, similar to DeviceIndex.

For network interfaces we have:

  • NetworkIndex, similar to ControllerIndex and TaskIndex
  • nwpluginID, similar to PluginID and CPluginID
  • networkDriver, similar to DeviceIndex and ProtocolIndex

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.
Suggestion for column names:

  • Nr
  • Enabled
  • Modbus Interface Type (can be left out as long as we only have Modbus RTU)
  • Connected (then show number of devices + list of IDs)
  • Port (the used serial port, baudrate and some extra GPIOs if used)

I think it is best to have a look at the NetworkPage how this is done using KeyValue types outside the KV-store.

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 8, 2026

Some extras....

id_to_match looks like a "magic number" to check if the data in the file really matches the intended module. The network plugin identifier seems to be used for network plugins.

Yep, that's to check whether it makes sense to load the data.
For example if that blob of data stores data for NW005 (PPP modem) where we expect NW003 (RMII Ethernet), then there is no need to attempt to read the data.
There is also a version number included in this, which allows us to later change what is stored and maybe convert older settings into newer settings. So far this version number is not yet used, but I added it into the header for future use.

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 :)

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 8, 2026

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.
Even if we would later add Modbus over IP and need to store a hostname + port.

@flashmark
Copy link
Copy Markdown
Contributor Author

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.

@TD-er
Copy link
Copy Markdown
Member

TD-er commented Apr 9, 2026

I think the following is needed:

  • serial port type: 1 byte (there is an enum for that in ESPEasySerial)
  • RX pin: int8_t
  • TX pin: int8_t
  • DE/RE pin(s): 2x int8_t
  • Baudrate: uint32_t
  • Timeout: uint32_t (or 16 bit? 65 sec is more than enough)
  • Serial specific config like nr bits (3b), start bit (1b), stop bits (2b), parity (1b): uint8_t
  • Checkbox to enable collision detection (ESP32-only feature and requires /RE pin be connected to GND)

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)
And when reading settings, you can read a KV-tuple without knowing the exact size, which allows to dynamically change the size of a tuple later without adding a lot of conversion code. For example if you start with a 16-bit timeout now and for some reason later we decide it needs to be an uint32_t, you can simply change it and the next time it is saved it will be stored as such without rendering older settings incompatible.
That's why I created this KV-store, as we constantly ran into these kinds of issues which required extra code to convert older settings.

#if FEATURE_STORE_NETWORK_INTERFACE_SETTINGS
case Enum::NetworkInterfaceSettings_Type:
#endif
case Enum::ModbusInterfaceSettings_Type:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an #if FEATURE_MODBUS_FAC / #endif is missing here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/src/Helpers/Modbus_link.cpp
Comment thread src/src/Helpers/Modbus_mgr.h Outdated
Comment thread src/src/Helpers/Modbus_mgr.h Outdated
Comment thread src/_P183_modbus.ino

case PLUGIN_WEBFORM_LOAD:
{
addFormNumericBox(F("Modbus Link"), P183_LINK_ID_LABEL, P183_LINK_ID, 0, 3);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different for ESP8266 and ESP32.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@TD-er TD-er Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here...

ESPEasySerialPort ModbusLINK_struct::getPort(void) const
{
return ESPEasySerialPort();
return ESPEasySerialPort();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

@tonhuisman tonhuisman Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (isInitialized()) {

@@ -152,7 +145,7 @@ uint16_t P183_data_struct::readRegisterWait(uint16_t address) {

while (state == ModbusResultState::Busy) {
delay(50);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, delays should be avoided like the plague, processCommands() should be called in 50/sec or 10/sec events instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants