Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature: Modbus Support #1744

Closed
proddy opened this issue May 6, 2024 · 39 comments
Closed

Feature: Modbus Support #1744

proddy opened this issue May 6, 2024 · 39 comments
Labels
enhancement New feature or request
Milestone
v3.7.0

Comments

@proddy
Copy link
Contributor

proddy commented May 6, 2024 •
edited
Loading

Creating this issue to track issues on the Modbus feature.

@mheyse did all the coding, in this PR #1618

Code is in this branch: https://github.com/emsesp/EMS-ESP32/tree/feat_modbus
@proddy proddy added the enhancement New feature or request label May 6, 2024
@proddy proddy added this to the v3.7.0 milestone May 6, 2024
@proddy
Copy link
Contributor Author

proddy commented May 6, 2024

I've added the Web settings.

Also noticed with modbus enabled it takes up 17kb of heap memory, and 13 when disabled. We need to try and bring that back. I haven't looked into all the code yet.

proddy added a commit that referenced this issue May 6, 2024
@proddy
modbus webui #1744
6fd8fab
@mheyse
Copy link
Contributor

mheyse commented May 6, 2024

How exactly do you compare the heap usage? Build the firmware from dev and from feat_modbus and compare System -> System Memory -> Heap? Or is there a better way?

I suspect that the data I allocate in modbus_entity_parameters.cpp is not stored in Flash (as I thought it would) if it takes up 13kB of heap memory even when disabled.

There are other optimizations I can do (e.g. I register 4 modbus workers each for all ems devices, this could be changed to use ANY_SERVER and ANY_FUNCTION_CODE wildcards)

@proddy
Copy link
Contributor Author

proddy commented May 6, 2024

yes, I actually use a python script for memory profiling but you can also use the WebUI or just read the "free mem" from the JSON output of http://ems-esp.local/api/system

The Modbus is a great feature and easily fits on modern ESP32s like the S3, but it will struggle on 4MB variants. We just need to find ways to reduce the amount of heap memory if Modbus isn't been used. When it's enabled it's fine as that is the price to pay for a new module/feature.

I see the object ModbusServerTCPasync instantiated as default but this can't be the main culprit, something else is eating the memory, even when it's disabled.

@proddy
Copy link
Contributor Author

proddy commented May 7, 2024

could you possibly instantiate ModbusServerTCPasync from within Modbus::start() and just use static pointers? This may save on heap memory.

Also check that LOCAL_LOG_LEVEL is not set, unless EMSESP_DEBUG is.

I'm working on a new C++ Factory class so we can try this as a hot-pluggable module. Could you describe which integration points you have with EMS-ESP? I see the start() but you have nothing in the loop() ?

@proddy
Copy link
Contributor Author

proddy commented May 9, 2024

dev has 190KB free heap, feat_modbus with Modbus disabled and using the Michael's change to static Modbus * modbus_ is 177KB so we're still eating up 13KB of memory even if not using Modbus. We need to bring this down before accepting into the dev branch. Can you see what you can do @mheyse ?

@proddy proddy removed this from the v3.7.0 milestone May 9, 2024
@mheyse
Copy link
Contributor

mheyse commented May 9, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented May 10, 2024

The design pattern is that Modbus should allocate as little to no heap memory if the service is not enabled. So something is still lurking in the headerfiles and eating up the heap during runtime (not static, progmem/flash). I still suspect it's the creation of the ModbusServerTCPasync object in modbus.cpp - see if you can turn that into a pointer and new-it when it needs to be created.

@mheyse
Copy link
Contributor

mheyse commented May 10, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented May 10, 2024 •
edited
Loading

that's probably it - missed that. The modbus_register_mappings list and tag_to_type map. Is there a way to populate both these in the start() function using static_ptr? Also watch out for that Serial.print() in the start() function - sending Serial on a live EMS-ESP can cause havok since we're spoofing the UART for the EMS bus.

even better is to make it dynamic, like we do with HA MQTT Discovery so the modbus registry's are only created when a valid ems device entity is found.

mheyse pushed a commit to mheyse/EMS-ESP32 that referenced this issue May 18, 2024
@proddy @vrcmh
modbus webui emsesp#1744
b06b152
@mheyse
Copy link
Contributor

mheyse commented May 25, 2024

FYI, I submitted another PR addressing the memory iasues: #1770

@proddy
Copy link
Contributor Author

proddy commented May 28, 2024

thanks, I'll take a look and run some more benchmarking. I've been out the last weeks. I still need to fix the Web settings right?

@mheyse
Copy link
Contributor

mheyse commented May 29, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jun 4, 2024

I made some small changes to the branch to fix the UI settings.

Your latest changes are good with memory
without modbus code its 195 / 107 (free heap / max alloc)
with modbus disabled its 192 / 107
with modbus enabled its 187 / 107

So this feature only takes up 3kb extra, which is acceptable.

@proddy proddy added this to the v3.7.0 milestone Jun 4, 2024
@mheyse
Copy link
Contributor

mheyse commented Jun 4, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jun 4, 2024

Is there a way to add the Modbus registers dynamically as soon as they show up, like we do with MQTT Discovery?

@mheyse
Copy link
Contributor

mheyse commented Jun 4, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jun 5, 2024

Ok, I understand. I'll see if I can run the generation of modbus_entity_parameters.hpp as part of the platformio build target

@proddy
Copy link
Contributor Author

proddy commented Jun 9, 2024

I looked into automating this today, but it wouldn't compile. Seems running the generation of src/modbus_entity_parameters.hpp is not compatible with the modified class signatures. Can you take a look?

@proddy
Copy link
Contributor Author

proddy commented Jun 21, 2024

@mheyse any progress on this? It would be good to finally include it 3.7.0

@mheyse
Copy link
Contributor

mheyse commented Jun 21, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jun 25, 2024

Sorry to hear you were unwell. There's no rush - let me know how I can help

@mheyse
Copy link
Contributor

mheyse commented Jun 27, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jun 29, 2024

yes, I think it needs to be updated manually by running the script and the updated modbus_entity_parameters.hpp file checked in.

Is there some documentation you like to write, which explains what it does - which we can add to the wiki?

@proddy
Copy link
Contributor Author

proddy commented Jun 29, 2024

@mheyse I checked the code I think it's good to go. Can you refresh your local fork of the dev branch, then Git Merge your local copy of the feat_modbus branch? Then test and if it's good create a PR on dev. That way you'll own the feature and the PR.

@mheyse
Copy link
Contributor

mheyse commented Jun 29, 2024 via email

@mheyse
Copy link
Contributor

mheyse commented Jun 30, 2024 via email

@mheyse
Copy link
Contributor

mheyse commented Jul 1, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jul 1, 2024

Thanks for that - we'll review.

@proddy
Copy link
Contributor Author

proddy commented Jul 3, 2024

merged.

@proddy
Copy link
Contributor Author

proddy commented Jul 4, 2024

@mheyse - I'm getting compile errors when compiling with -DEMSESP_TEST. It happens here:

#if defined(EMSESP_STANDALONE) || defined(EMSESP_TEST)
#include <modbus_test.h>
#endif

perhaps a namespace clash with Error

@mheyse
Copy link
Contributor

mheyse commented Jul 5, 2024 via email

@proddy
Copy link
Contributor Author

proddy commented Jul 5, 2024

set my_build_flags = -DEMSESP_TEST in a pio_local.ini or change it directly in platform.ini and the compile breaks.

We use EMSESP_TEST for creating real ESP32 firmware for testing on ESP32's, not just in standalone mode.

@mheyse
Copy link
Contributor

mheyse commented Jul 6, 2024

Ah OK, I thought EMSESP_TEST is only used for testing on PCs. In this case the eModbus libraries are available, too, and || defined(EMSESP_TEST) can simply be removed.

Fix: #1853

@wint100
Copy link

wint100 commented Jul 9, 2024

Is there a standard Modbus register list available?

@mheyse
Copy link
Contributor

mheyse commented Jul 10, 2024

I created a PR with the docs here: emsesp/docs#36
It also documents the modbus registers.

@wint100
Copy link

wint100 commented Jul 22, 2024

I'm having issues writing to the 'Heating Temperature' using Modbus. I can read back all values, but when trying to write to Holding Register 14 with a uint16 value of 50, return an error on my Modbus Master. I've tried different Modbus Master hardware but both have the same result. Is there anything else I need to do when writing to a Holding Register?

@mheyse
Copy link
Contributor

mheyse commented Jul 23, 2024 via email

@wint100
Copy link

wint100 commented Jul 23, 2024

It appears to work on the S3 hardware but not on the older hardware. I have 20 of the S3 boards and these work well, but the older E32 boards (ESP32-D0WD-V3) don't seem to allow writing of the Heating Temperature.

Error from older board
2024-07-23 08:42:12.268 E 140: [modbus] Modbus write command failed with error: unrecognized path (Error)

@proddy
Copy link
Contributor Author

proddy commented Jul 25, 2024

This is in 3.7.0 now, so closing. Any issues/bugs can be reported using the normal channels.

@proddy proddy closed this as completed Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v3.7.0
Development

No branches or pull requests
3 participants
@proddy @mheyse @wint100