-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add support for acting as Modbus server #4874
Conversation
ee5107d
to
53a5888
Compare
53a5888
to
107826c
Compare
@JeroenVanOort This is great and exactly what I am talking about. I gave it a good look and pointed to a very few things but nothing important. A net of +126 lines of code and it is backwards compatible, which is very important. From reading the code, I understand writing registers is TBD. I hope to be able to give it a good testing next week. Cheers |
107826c
to
5580e3f
Compare
7325ab6
to
6af076e
Compare
a6c9205
to
d3036a3
Compare
Hey there @martgras, mind taking a look at this pull request as it has been labeled with an integration ( |
The code itself is ready for review: only need to make a PR for the docs. |
@@ -419,10 +435,14 @@ class ModbusController : public PollingComponent, public modbus::ModbusDevice { | |||
void queue_command(const ModbusCommandItem &command); | |||
/// Registers a sensor with the controller. Called by esphomes code generator | |||
void add_sensor_item(SensorItem *item) { sensorset_.insert(item); } | |||
/// Registers a server register with the controller. Called by esphomes code generator | |||
void add_server_register(ServerRegister *serverregister) { serverregisters_.push_back(serverregister); } |
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.
From my point of view we should not serverregister and instead maintain server registers in sensorset_, too.
This allows to inherit modbus::ModbusDevice e.g. in sdm_meter and set sensors for registers and use the role for server and client implementation.
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.
I think I understand what you're trying to say, but I think SensorItem
has some properties that are not needed for using it as a server register model and SensorItem doesn't have a lamba property for returning the register value.
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.
Yes that's the overhead if you implement both roles in one class. That's the reason for me to add a new component.
Your solution allows to inherit ModbusDevice e.g. in sdm_meter and add SensorItems and let use user choose the role.
My biggest objection on the current implementation is that it's only providing lambda to get the value, but from my point of view it should be possible to create a device in pure yaml without knowing any c++.
I see many people asking for virtual/fake modbus devices to create an adapter between devices in to solar area and that's how I currently use my modbus_device.
d3036a3
to
292ebd3
Compare
hey guys, any update on this? Really interested in this. |
Hi @JeroenVanOort, Thanks for your work on this! Please forgive me if I'm missing something, but after implementing your pull request, I think there's a bug in your on_modbus_read_registers function in the esphome/components/modbus_controller/modbus_controller.cpp file. I believe there's a mistake in the iteration logic? Again please correct me if I'm missing something, but it appears that the outer for loop will continue iterating through all the set registers even if the first one matches the request, and with "found" set to false on every iteration, each subsequent register will throw an error. Does the "found" flag only need to be set once, before the initial for loop? I think that would work in my use-case, but I don't know if it would break others that I don't understand. Thanks T Some context:
With this yaml, a request 0104000c0002b1c8 (2 registers starting at 0c, works fine). The same yaml without the comments gives this response:
|
What you probably want to do, is combine the controllers with the same address into one, like this:
The way you've done it, multiple devices are matched in |
292ebd3
to
b0f8aae
Compare
hi @JeroenVanOort, i also implemented your pr and i am a little bit confused. My config:
Log output:
So it says
but it responds much more registers.. And also the float to hex conversion does not work as i need it. Thanks so far for the great work! |
Hi @JeroenVanOort, i fixed an issue in JeroenVanOort#1. Now only the registers are returned for what was asked in the request. |
b0f8aae
to
bf21d07
Compare
679d771
to
abb6325
Compare
To be honest, I don't even know. It indeed makes way more sense to use
The way this is currently implemented, requesting 1 register at 0x9001 would get you both words (so 2 registers). This might not be great, but I personally don't care about it too much. Requesting 0x9002 would get you nothing, about which I don't care too much either, because I don't see a use case for requesting high and low words of the same value separately.
Thank you for acknowledging this! From time to time it feels like everyone is just nitpicking at my code while I'm just trying to contribute something that I made for myself. It takes conscious thought to realize that we are all just trying to make good things. |
We're all trying to team up our efforts to make it better for everyone. I'm sorry if I made you feel uncomfortable, it's also probably because English is not my native language. |
😎
Ok, I think that's fine. Perhaps responding with an error might be better than nothing, but that can wait for another PR, I'm sure, as could responding with the requested number of registers. Not sure how compliant it is to respond with more than requested, but it's probably not a big deal.
I know that having people make too many comments can feel like they are never satisfied. I'm sorry if I made you feel that way. Thanks again for your contribution! |
Hope this gets merged into a Release soon. Been using this feature for a few months now, works great! |
I'm really keen to try it but don't know how to, I've installed ESPhome (dev), but I can't get this pull to work. × This environment is externally managed FATAL: Failed installing ESPHome from fork. Any ideas what I'm doing wrong? Many thanks |
For what it's worth, I've tested this between two Kincony boards (one KC868-AI acting as a server, exporting the state of the dry contact switches, and one KC868-A16 acting as a client reading those states every second) and it works wonderfully! Thanks for this! Hope to see this merged soon. |
Tested with 2024.04.0, working great. |
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
644f15f
to
96710aa
Compare
96710aa
to
c6dfcae
Compare
What does this implement/fix?
This adds the ability to let ESPHome act as a Modbus RTU server, i.e. it can handle requests from a modbus client instead of being the client itself.
Types of changes
Related issue or feature (if applicable): fixes esphome/feature-requests#708
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#3332
Test Environment
Example entry for
config.yaml
:Checklist:
tests/
folder).If user exposed functionality or configuration variables are added/changed: