feat: add modbus support

[mheyse]

[draft] - preview, not ready to be merged. TODOs see below.

This pull request adds Modbus support.

General

• Each device on the EMS bus is mapped to its own Modbus server. The server ID is the same as the device ID.
• There is one additional EMSESP Modbus server serving metadata like the number of devices and device IDs.
• The modbus server listens on port 502.

EMSESP Modbus server

There is one Modbus server for metadata. It currently transmits the following information:

• Register 1: The number of EMS devices detected on the bus.
• Registers 1000 + n: The ID of the nth device.
• Default server ID is 1

Modbus register/entity mapping

• The entity/register mapping depends on the device type.
• The registers for each device are divided into blocks of 1000. Each block corresponds to an entity tag (as defined in DeviceValueTAG). So for example all values for heating circuit 3 are tagged with TAG_HC3. TAG_HC3 is defined as 7, so the hc3 values start at block 7, which starts at register address 7000. This works similar with all tags, also for example with TAG_DEVICE_DATA(3, register start address 3000)
• For each entity a register offset is defined. For example, entity ecotemp for device type thermosotat has the offset 5. So the Modbus address for ecotemp in hc2 (TAG_HC2 = 6) would be 6005.
• The values transmitted are the "raw" values, i.e. without applying the (potentially device dependent) numeric_operator. For example ecotemp for a RC310 thermostat has numeric operator DV_NUMOP_DIV2, so the value read from the register needs to be divided by 2 to get the actual temperature.
• Data types BOOL, INT, UINT, SHORT, USHORT and ENUM are transmitted in a single register, ULONG and TIME in two registers (in big endian order) and STRING as an entity dependent number of registers, packing two characters in a 16 bit register and including the terminating \0.

TODOs

[x] The implementation is currently read-only. Implement writing.
[x] performance optimizations
[x] memory optimizations (currently the mapping uses 44kB of RAM) Fixed in edc36cf
[ ] documentation
[ ] configuration UI

[proddy]

Hi @mheyse - I looked through the code the best I can and it looks solid. A few points:

• Could we eventually wrap all the Modbus into #define's so we can have CI do a special build for ESP32S3's (or any dual-core ESP32s with >4MB)
• A test to simulate would be handy for bug fixing (for those without a Loxone or similar). I've seen Node-Red examples and also this one using docker: https://github.com/cybcon/modbus-server
• As we continue to add things to the dev branch make sure it's kept in sync. If you prefer we can also make a specific branch for the modbus work.

[mheyse]

Hi,

thanks for the review.

• Could we eventually wrap all the Modbus into #define's so we can have CI do a special build for ESP32S3's (or any dual-core ESP32s with >4MB)

Not sure when you looked at it, maybe my last commit was not in there yet? In this commit I changed the configuration from using a std::map to just an initializer list (to save on RAM), and all registers are defined using the REGISTER_MAPPING #define, e.g. REGISTER_MAPPING(dt::BOILER, TAG_TYPE_BOILER_DATA_WW, FL_(nrgWw), 0, 2). Is this sufficient?

• A test to simulate would be handy for bug fixing (for those without a Loxone or similar). I've seen Node-Red examples and also this one using docker: https://github.com/cybcon/modbus-server

What exactly would such a test look like? Are you talking about automated tests that run during a CI build, or something "hands-on"?

Currently I use a very simple C# program (using the EasyModbusTCP library) to connect to the EMS-ESP and read out some values. "cybcon/modbus-server" is a server, wouldn't we need a client for such a test? There's also the pyModbusTCP (haven't looked at it yet) that should allow to easily implement a Modbus client in Python. Would this be something of a test?

• As we continue to add things to the dev branch make sure it's kept in sync. If you prefer we can also make a specific branch for the modbus work.

I can keep working in my repo, and I'll keep it in sync with the dev branch.

I have one more question regarding addressing the individual devices: The HTTP API and the MQTT connector access devices by device type and not the device address or id (which is what I implemented in the Modbus connector) - the API path is something like thermostat/hc1/seltemp. Do you think it makes sense to change that in the Modbus connector, too, to be consistent? Then the server ID would be the device type and not the device id any more. (I don't know too much about the EMS bus - but is it impossible to have multiple devices of the same type on one bus? This is what addressing by device type would imply).

[mheyse]

Hi,

the Modbus interface currently seems to do what I want it to do - reading and writing entities works in my tests as expected. The two big things missing are

• configuration UI
• documentation

Re. configuration UI: Could you give me a hint what's the mimimum that needs to be implemented to have a simple on/off switch for the Modbus component? Looking at the mqtt configuration user interace code it looks pretty involved. There's only very few parameters that can be configured (enabled, TCP port, max. num of clients, timeout).

Re. documentation: Where is it stored, how do I contribute? I'd like to generate a Modbus register doc from dump_entities.csv automatically, and additionally write a bit about the interface in general.

Thanks!
Michael

[proddy]

This is going to be an awesome addition, nice work!

• I suggest me, and Michael does a code review to familiarize ourselves with how it works and spot any potential gotchas (like memory or timing issues as we've had our fair share over the years!). I did have problems compiling (error was error: use of 'auto' in lambda parameter declaration only available with -std=c++14 or -std=gnu++14)
• We need to test against all variations of the ESP32, with 4MB to 16MB flashes to see how it performs
• I can build the UI for you if you tell me which parameters are configurable. If it's just enable/disable we can add it to the Application Settings page.
• Documentation is another public GH repo (https://github.com/emsesp/docs) so feel free to hack away when its final

[mheyse]

Can you tell me where you get the compile error, is it in test.cpp? I assign lambdas to auto vars there, but when I build here (I usually do a ci build, pio run -e ci or a pio run -e standalone to run the test modbus command or dump entities) it compiles without issues, and compiling with -v also states it's using --std=c++11 with the xtensa-esp32-elf-g++ compiler. I'm on a Mac.

I replaced the auto lambdas in test.cpp with explicit std::function types, could you please try again? And if you still get errors please give me the exact locations.

[mheyse]

Regarding the config UI: The parameters that should be configurable are defined at the beginning of modbus.cpp:

// parameters
bool     Modbus::modbus_enabled_ = true;
uint8_t  Modbus::systemServerId_ = 1;
uint16_t Modbus::port_           = 502;
uint8_t  Modbus::maxClients_     = 10;
uint32_t Modbus::timeoutMillis_  = 10000;

Configuring systemServerId_ probably makes no sense, it should always be 1. And then I'd need to add methods to start and stop the modbus server when it's enabled or disabled - currently it is always started by calling Modbus::start() from EMSESP::start().

[proddy]

I see the issue, the platformio ci target it misses setting the build_flags (this is a mistake!) which sets -std=gnu++17. auto lambdas are so more preferred over std::function if possible.

EDIT forget that, the gn++17 is included. And it compiles fine on my MacBook, just not on Linux/Ubuntu.

As for settings, we can add to Settings like we do with SysLog and just set enabled/disabled, port, maxClients and the timeout?

[mheyse]

So now I'm confused does it compile or not? :-) I can try it on Ubuntu, too, if needed.
Then I'll reverse the auto -> std::function commit?
Re. settings, yes, that's the parameters we need.

[proddy]

there's a typo at

EMS-ESP32/platformio.ini

Line 46 in 14af4a0

build_unflags = ${common.unbuild_flags}s

with the extra "s" which is preventing the lolin_s3 build from compiling. I'll fix in dev.

[proddy]

For the settings, create modbus_ and add to system.h, WebSettingsService.cpp/.h, system.cpp to read and set the values. The easiest way is to copy how we did it for Syslog. Just search for
syslog_mark_interval, syslog_host or syslog_port.

Once they are settings variables, I can build the WebUI section.

Some memory comparisons (flash/ram and build and heap/max alloc mem during runtime) on ESP32-S3 and a normal ESP32 would be useful to see.

[proddy]

@mheyse if you want some help with the Settings let me know

[mheyse]

I didn't have time to look at it until now - I had to move to a Windows PC from Mac, and now I can't get the firmware to build at all any more, I always get a lib/framework/ESP8266React.cpp:3:10: fatal error: WWWData.h: No such file or directory error. At first I thought it's because pre:scripts/build_interface.py was commented out in commit dc68258 but reverting it does not help. I found hints on the net that it may be caused by an outdated nodejs, but that's not the reason here.

I also tried building in WSL2/Ubuntu, same error.

Any hints?

>pio --version
PlatformIO Core, version 6.1.15

>node -v
v20.12.2

>npm -v
10.5.0

>yarn -v
1.22.22

I added the parameters and treated the "enable" flag analogous to the telnet server - so a reboot is required if Modbus is enbabled or disabled. But because I can't build I have no idea if it works.

[mheyse]

OK I added pre:scripts/build_interface.py to the extra_scripts in the [env:ci] section I'm usually building and now it works again, so something seems to be broken in platformio.ini. (A plain pio run withut specifying an env explicitly using the default env also failed)

[proddy]

pio run works for me, out the box. Do you have a custom pio_local.ini that is overriding your settings?

[mheyse]

no I don't. FWIW, I first ran pio run -e ci and after that failed pio run.

I pushed my changes after rebasing to current dev.

One question: you removed DeviceValue::TAG_HEARTBEAT. Regarding Modbus this is a breakting change because it changes the values of all tags, and I'm using "numeric tag id" * 1000 as the base address for the register block. So when a tag is deleted the values of all tags behind it change in the enum and all registers are shifted by 1000.

Are changes like that expected to occur more frequently? If yes I'd rather hardcode fixed values for the tag -> modbus register block mapping.

[mheyse]

(same with the EMSdevice:DeviceType)

[proddy]

we made some major breaking changes with the tags, and this will be the last change. If you point us to the code that uses the tags myself and @MichaelDvP can review.

For building, I wouldn't use the target ci as this is for GitHub Action scripts. Use the board you're building for, e.g. lolin_s3 or esp32_4M

[mheyse]

Yes, it should be a esp32_16M (I'm using a BBQKees E32 Gateway V2).

Regarding the enums I'm using:

• The DeviceType is mapped directly to the Modbus server ID (as defined in Modbus::modbus_register_mappings in file modbus_entity_parameters.cpp which itself can be generated from a dump_entities.csv using scripts/update_modbus_registers.py
• I use the DeviceValueTAG enum to define the DeviceValueTAGType enum (in modbus.h). This defines the Modbus register block - so for example the heating circuits are mapped to the base register address REGISTER_BLOCK_SIZE * DeviceValueTAGType::TAG_TYPE_HC, and TAG_TYPE_HC is defined as DeviceValue::DeviceValueTAG::TAG_HC1. REGISTER_BLOCK_SIZE is defined as 1000. HC1 is directly at that register address, HC2 at that address + REGISTER_BLOCK_SIZE and so on.
• The handleRead and handleWrite methods in modbus.cpp
  • map a register address to a tag (by dividing it by REGISTER_BLOCK_SIZE rounded down,
  • calculates the offset register_offset within the register block
  • then looks up the tag type in the Modbus::tag_to_type map (defined in modbus.cpp).
  • It then performs a binary search on Modbus::modbus_register_mappings using the combination of device_type, tag_type and register_offset as a key to find the entity short name associated with the register.

[mheyse]

I updated update_modbus_registers.py and modbus_entity_parameters.cpp to reflect the changes in EMSdevice::DeviceType and DeviceValue::DeviceValueTAG. After changing the modbus registers in my KNX setup, too, everything seems to be working again.

(FYI: I still had to re-enable the pre:scripts/build_interface.py in section [espressi32_base] to build for esp32_16M. I also got some errors about potentially uninitialized values when building standalone on Linux. I attached a patch with all I needed to change to make it build in both standalone and esp32_16M envs.
0001-Fix-build-issues.patch
)

[proddy]

there was one change in emsdevicevalue.h to remove TAG_HEARTBEAT (it's hardcoded now as its only used in one place) and make the tags match the heating circuits. So TAG_HC1 is now 1. Before it was 3 and we had to do some unneeded subtractions everywhere. Apart from that it's the same so you should be able to adjust your code.

[MichaelDvP]

Please check the types, DeviceValueTag has changed from uint8_t to int8_t to match the command id.
We have now: 0- no tag, >0 tag, <0 wildcard

Once the webpage is created there is no need to build it every time, Saves a lot of time when compiling code. To build it once manually:
cd interface
yarn install (or yarn up, only if package list has changed)
yarn typesafe-i18n --no-watch (only if i18 indices are changed)
yarn build
yarn webUI

[mheyse]

Thank you both for your infos. I updated all code yesterday with the settings changes so please feel free to take a look, I think you could implement the UI now @proddy.

@MichaelDvP I still get the following compile errors on Ubuntu, gcc 1.4.0, if I don't apply above patch (sans the platformio.ini change):

src/emsdevice.cpp: In member function ‘bool emsesp::EMSdevice::is_readonly(const string&, int8_t) const’:
src/emsdevice.cpp:723:25: error: enumerated and non-enumerated type in conditional expression [-Werror=extra]
  723 |     int8_t tag = id > 0 ? id : DeviceValueTAG::TAG_NONE;
      |                  ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
*** [.pio/build/standalone/src/emsdevice.o] Error 1
src/devices/boiler.cpp: In member function ‘void emsesp::Boiler::process_UBAFactory(std::shared_ptr<const emsesp::Telegram>)’:
src/devices/boiler.cpp:1170:19: error: ‘max’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1170 |         set_minmax(&selBurnPow_, 0, max);
      |         ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
src/devices/boiler.cpp:1169:19: error: ‘min’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
 1169 |         set_minmax(&wwMaxPower_, min, max);
      |         ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~
In file included from src/emsesp.h:54,
                 from src/devices/boiler.h:22,
                 from src/devices/boiler.cpp:19:
src/emsdevice.h:153:25: error: ‘nomPower’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  153 |             value       = newvalue;
      |             ~~~~~~~~~~~~^~~~~~~~~~
src/devices/boiler.cpp:1158:23: note: ‘nomPower’ was declared here
 1158 |     uint8_t min, max, nomPower;
      |                       ^~~~~~~~
cc1plus: all warnings being treated as errors
*** [.pio/build/standalone/src/devices/boiler.o] Error 1

[mheyse]

FYI, when clone the doc repo I get the following warning:

warning: the following paths have collided (e.g. case-sensitive paths
on a case-insensitive filesystem) and only one from the same
colliding group is in the working tree:

  'docs/_media/screenshot/oh_inbox.PNG'
  'docs/_media/screenshot/oh_inbox.png'

[proddy]

I'll fix those lint errors. You not be sync'd with the latest dev? you shouldn't be seeing those compile warnings

[proddy]

actually @mheyse since this is such a big change, and a major feature, I'll create a new feature branch. Then we can work together on that. Can you create a PR against the branch feat_modbus?

https://github.com/emsesp/EMS-ESP32/tree/feat_modbus

[mheyse]

I'll fix those lint errors. You not be sync'd with the latest dev? you shouldn't be seeing those compile warnings

It was 2 days old :-) I just rebased to the latest dev, and the build warnings/errors are gone, thanks.

EDIT: sorry that was too quick. I still get the errors when building standalone on Ubuntu with gcc 11.4 because of the -Werror=maybe-uninitialized flag. I still need this change:

---
 src/devices/boiler.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/devices/boiler.cpp b/src/devices/boiler.cpp
index c6b567dc..49da236f 100644
--- a/src/devices/boiler.cpp
+++ b/src/devices/boiler.cpp
@@ -1155,7 +1155,7 @@ void Boiler::process_UBAFactory(std::shared_ptr<const Telegram> telegram) {
         return;
     }
     toggle_fetch(telegram->type_id, false); // only read once
-    uint8_t min, max, nomPower;
+    uint8_t min = 0, max = 0, nomPower = 0;
     telegram->read_value(nomPower, 4);
     telegram->read_value(min, 5);
     telegram->read_value(max, 6);
-- 
2.44.0.windows.1

[mheyse]

actually @mheyse since this is such a big change, and a major feature, I'll create a new feature branch. Then we can work together on that. Can you create a PR against the branch feat_modbus?

https://github.com/emsesp/EMS-ESP32/tree/feat_modbus

I changed the base branch of this PR to the feat_modbus branch.

Because there are so many commits and WIP changes I think it would make sense that I squash everything I did to a single commit, also makes it easier to review and rebase. Do you agree?

[proddy]

yup, squish it!