-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat: Modbus support multiple slaves (gateway feature) #9279
Conversation
I'm testing this now in my configuration and so far I haven't found any problems. The configuration was straight-forward - I figured it out from only the sample config (which means the sample is solid). The only problem I had was not specifying I have two minor comments that you should feel free to ignore
I'm not certain you really need this. I feel like you could infer it from the presence of an non-empty [[inputs.modbus.request]] array. I also feel like you can pretty easily merge the old and new configurations if you wanted to. If you did so, then requests in the "original" section would just assume the default unit address. This isn't really a super big deal, except that getting rid of configurations you don't really need is always good.
I think this is controller because that's how it appeared in the original modbus input, but if you're in a gateway configuration the more appropriate term is "gateway." As a compromise, would you be willing to change it to accept either "controller" or "gateway"? or "url" ? (Maybe that's the thing to do - change it to url, then for backward compatibility silently accept "controller". It really think that thinking of it as a URL is sound, and URL is a term that applies whether you're talking directly to a controller or to a controller via a gateway). No complaints so far, if you have time give me a little while to run it and see what happens. |
I'm testing this in as many ways as I can reasonably think of using real-world hardware. One of the devices I'm trying to use is an Eaton (Cutler-Hammer) EasyE4, a very inexpensive nano plc. This particular plc has 32-bit registers that overlap the 16-bit registers in pairs. It's not unusual, but depending on the logic you write it's likely you'll have a mix of 16-bit and 32-bit registers in adjacent addresses. That caused a problem to bubble to the surface. I ran into a problem with certain byte order specifications. It really occurs if you mix 16 and 32 bit mappings in the same request. I tracked this down, and I know why it's doing it. Here is the configuration:
This yields the following error:
The place where it is failing is right here, because CDAB isn't defined:
I know this is confusing, but I can explain it. The problem is that for a 16-bit register, "ABCD" and "CDAB" are the same thing, because either the "AB" part or the "CD" part gets used. This becomes an issue if you're trying to do a mixture of 16 and 32 bit mappings in a single request - which should be allowed, I think, though I admit it's perhaps a bit unusual. I can work around by splitting it into two separate requests. In my original attempt to support multiple unit addresses I had rewritten all of this code. What I tried to do was to generalize it rather than having a selection of possible byte orders. See here and here. You can argue that everything I did was unnecessary - that it doesn't need to be generalized ... but according to a comment I made in my unit test I think I thought of this:
so the clue there is that for this to work format CDAB explicitly states a 32-bit conversion but it implies an 16-bit and 64-bit conversion. I think we need to keep that. I think there are two options to fix this:
At this point, I'm not going to say I trust my generic converter without more eyeballs on it, so I'd lean toward just fixing the immediate problem of allowing the 32-bit order specifiers to imply either AB or BA if it's a 16-bit register. I don't want to let idealism be the enemy of the good enough.
|
Hey @wz2b, thanks for testing! Let me first explain one thing regarding field handling and then I try to answer your comments. The modbus plugin as is, reads the words from the device specified by the request (actually the range from start to end). It stores those words in a byte-array without any modification, i.e. the mentioned byte-array has the raw bytes in the order as delivered by the device. Coming to the error you get: |
It makes sense to me. My thought process is that ABCD and CDAB are abbreviated ways of saying these two things:
WIthout that implication it is impossible to fetch both 16 and 32 bit values in the same request if the byte order is "AB" but the word order is CDAB". I think that's part of the problem - it's named "byte order" but really it's byte order and word order. Now, if you had separate parameters "byte_order" and "word_order"
No, I do not expect that ABCD only means 32-bit ordering. I expect that it means "strictly big endien, regardless of length." I think you actually think this, too:
there, you allow someone to use ABCD to specify a 64-bit value. So ABCD implies ABCDEFGH. You just didn't allow the same implication in the downward direction. Currently there is no option other than to split the request in two. So the way I see it, the easiest option is to do one of these things:
I understand your reasoning here. My choice still would be to deal with the ordering generically rather than with hard-coded cases, because I don't want to have to go back to add cases we didn't think of later.
I agree, that would be pretty absurd. However what we're talking about here isn't really "byte order" it's "word order" i.e. "how do you assemble modbus registers (words) into bigger representations." This PLC I'm testing with doesn't have 64-bit numbers so it's a little hard to imagine what they would have done but if I had to guess it would have been CDABGHEF because that would follow the pattern they used for low-part-first. I suppose my suggestion is Allow CDAB to imply AB in |
Hey @wz2b, after having almost finished a two-page reply to your post, I now see your point and see my mistake. :-) Sorry for the noise... If so, I'll add |
Yeah. You just said the whole thing I took two pages to type but you said it in one sentence. That's really it. I'm fine with solving it that way, as soon as new artifacts are available I'll resume testing. |
OK, mixing 16-bit and 32-bit requests working now! |
I just discovered that if you don't specify register_type it crashes:
The configuration that caused this:
My suggestion is that rather than make it required you default it to "holding" since that's what's most common. |
This is just food for thought. If you don't specify a slave address, the slave address defaults to 0. From the spec: On TCP/IP, the MODBUS server is addressed using its IP address; therefore, the MODBUS Unit Identifier is useless. The value 0xFF has to be used. When addressing a MODBUS server connected directly to a TCP/IP network, it’s recommended not using a significant MODBUS slave address in the “Unit Identifier” field. In the event of a re-allocation of the IP addresses within an automated system and if a IP address previously assigned to a MODBUS server is then assigned to a gateway, using a significant slave address may cause trouble because of a bad routing by the gateway. Using a nonsignificant slave address, the gateway will simply discard the MODBUS PDU with no trouble. 0xFF is recommended for the “Unit Identifier" as nonsignificant If in your request configuration you don't specify a slave address, then the common interpretation is that you mean to talk to the gateway itself, i.e. it isn't really a gateway it's just a device. Based on _ The value 0xFF has to be used_ it seems like defaulting to 0xFF is more correct; but then later on it says The value 0 is also accepted to communicate directly to a MODBUS/TCP device which would imply either is OK. |
I have a minor complaint about tags.
For the last, I have a specific use case. I have a bunch of sensors that measure various gases. I have four oxygen sensors, 11 carbon monoxide sensor, and some other number of hydrogen/propane/methane/etc. For each sensor I want to output
unless there's already a way to have per-request tags and I don't know what it is. |
I agree and will fix it. Thanks for testing this thoroughly! Regarding the |
You can still drop the type tag using
I agree that
Currently it is not possible. but we should definitively add it on the request level. I also have a use-case (similar to yours), but I don't want to fold it into this PR as it is massive already.
I'm currently doing it using a processor deciding which tags to add by evaluating the |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Yup. I can live with that. |
I'm running this in a production environment. When there are multiple requests with different slave ids there is an i/o timeout. I put a |
We need to get to the bottom of this. Is it possible you have responses arriving out of order? Are your devices slow to respond, or do their response times differ greatly? |
When there are multiple read shortly after each other it seems like the tcp connection gets closed, possibly from the modbus gateway device. I am using a waveshare rs485 to ethernet converter. Read the comment by gq0 on 7 Jan 2019 here https://github.com/grid-x/modbus/issues/11 where he recommend a >= 64ms delay when reading from a SolarEdge inverter. |
@mbezuidenhout we also saw this with other (mostly serial) devices. PR #9256 introduces workarounds, so you can set [[inputs.modbus]]
...
[inputs.modbus.workarounds]
pause_between_requests = "100ms"
The mentioned PR needs to be merged before this one anyway. |
I have another feature request, for this or the next version ... working on a PM800 and the power factor registers has an offset of 1, might be nice to have 'offset' (defaulted to 0.0) in addition to 'scale' in the polling configuration. |
@wz2b good point. Could you please open an issue for both, the offset feature and the tag-stuff we discussed above in order to not forget about those useful things!? |
688c970
to
40847d8
Compare
📦 Looks like new artifacts were built from this PR. Expand this list to get them here ! 🐯Artifact URLs |
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.
Thanks for driving this home!
Hi, great work! I would like to use this feature to request 2 modbus-slaves via one TCP/IP gateway, using a Docker image. However when I try to use it I get this error message:
I that because this code is not yet included in the "latest" official Docker image (it says the version is 1.20.4)? This is the modus-part set-up in my telegraf.conf I'm using to read out 2 SMD630 modbus utility meters that are connected to an TCP/IP gateway:
|
Hey @bikeymouse, this PR is only included in 1.21, so you need at least one of the release-candidates of that version (which is not yet released IIRC). This will solve the error you mention. Furthermore, your address-specification will not work with this type of specification as you now only need to specify the _start_address, i.e. [[inputs.modbus]]
name = "ModbusGateway"
timeout = "2s"
controller = "tcp://192.168.1.170:520"
transmission_mode = "RTUoverTCP"
[[inputs.modbus.request]]
slave_id = 3
measurement = "Solar"
byte_order = "ABCD"
register = "input"
fields = [
{ address=0, name="VoltageL1", type="FLOAT32-IEEE"},
{ address=2, name="VoltageL2", type="FLOAT32-IEEE"},
{ address=4, name="VoltageL3", type="FLOAT32-IEEE"},
{ address=6, name="CurrentL1", type="FLOAT32-IEEE"},
{ address=8, name="CurrentL2", type="FLOAT32-IEEE"},
{ address=10, name="CurrentL3", type="FLOAT32-IEEE"},
]
[[inputs.modbus.request]]
slave_id = 2
measurement = "Charger"
byte_order = "ABCD"
register = "input"
fields = [
{ address=0, name="VoltageL1", type="FLOAT32-IEEE"},
{ address=2, name="VoltageL2", type="FLOAT32-IEEE"},
{ address=4, name="VoltageL3", type="FLOAT32-IEEE"},
{ address=6, name="CurrentL1", type="FLOAT32-IEEE"},
{ address=8, name="CurrentL2", type="FLOAT32-IEEE"},
{ address=10, name="CurrentL3", type="FLOAT32-IEEE"},
] |
Hi @srebhan thanks so much for you good work and pointing me in the right direction. I'd like to start using this a.s.a.p. As I don't see any previous release-candidates in Docker hub perhaps that is not done and I have to wait for the 1.21 official release. |
@bikeymouse just clone this repo, install a recent golang on your machine and type |
Hi, thanks again! However when I start Telegraf with the config you posted above, I get this error:
This is at the part that starts with:
Any idea what could cause this? |
@bikeymouse let's not hijack this PR. Can you please open an issue and attach your |
You're right, sorry. I did found the reason (not to use FLOAT32-IEEE, but FLOAT32 instead). |
* origin/master: (133 commits) chore: restart service if it is already running and upgraded via RPM (influxdata#9970) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10237) fix: Handle duplicate registration of protocol-buffer files gracefully. (influxdata#10188) fix(http_listener_v2): fix panic on close (influxdata#10132) feat: add Vault input plugin (influxdata#10198) feat: support aws managed service for prometheus (influxdata#10202) fix: Make telegraf compile on Windows with golang 1.16.2 (influxdata#10246) Update changelog feat: Modbus add per-request tags (influxdata#10231) fix: Implement NaN and inf handling for elasticsearch output (influxdata#10196) feat: add nomad input plugin (influxdata#10106) fix: Print loaded plugins and deprecations for once and test (influxdata#10205) fix: eliminate MIB dependency for ifname processor (influxdata#10214) feat: Optimize locking for SNMP MIBs loading. (influxdata#10206) feat: Add SMART plugin concurrency configuration option, nvme-cli v1.14+ support and lint fixes. (influxdata#10150) feat: update configs (influxdata#10236) fix(inputs/kube_inventory): set TLS server name config properly (influxdata#9975) fix: Sudden close of Telegraf caused by OPC UA input plugin (influxdata#10230) fix: bump github.com/eclipse/paho.mqtt.golang from 1.3.0 to 1.3.5 (influxdata#9913) fix: json_v2 parser timestamp setting (influxdata#10221) fix: ensure graylog spec fields not prefixed with '_' (influxdata#10209) docs: remove duplicate links in CONTRIBUTING.md (influxdata#10218) fix: pool detection and metrics gathering for ZFS >= 2.1.x (influxdata#10099) fix: parallelism fix for ifname processor (influxdata#10007) chore: Forbids "log" package only for aggregators, inputs, outputs, parsers and processors (influxdata#10191) docs: address documentation gap when running telegraf in k8s (influxdata#10215) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10211) fix: mqtt topic extracting no longer requires all three fields (influxdata#10208) fix: windows service - graceful shutdown of telegraf (influxdata#9616) feat: update etc/telegraf.conf and etc/telegraf_windows.conf (influxdata#10201) feat: Modbus support multiple slaves (gateway feature) (influxdata#9279) fix: Revert unintented corruption of the Makefile from influxdata#10200. (influxdata#10203) chore: remove triggering update-config bot in CI (influxdata#10195) Update changelog feat: Implement deprecation infrastructure (influxdata#10200) fix: extra lock on init for safety (influxdata#10199) fix: resolve influxdata#10027 (influxdata#10112) fix: register bigquery to output plugins influxdata#10177 (influxdata#10178) fix: sysstat use unique temp file vs hard-coded (influxdata#10165) refactor: snmp to use gosmi (influxdata#9518) ...
resolves #7523
related to #8013
This is a PR implementing the support for multiple slave devices for the same connection. This is required for devices with limited connectivity (e.g. limiting the concurrent connections to 1 or a small number) but a high number of slave devices. These class of devices become more popular especially with ModbusTCP gateway hardware.
Additionally to supporting multiple slaves this PR implements a second (independent) configuration scheme to configure the reading on a per-request style. This is especially helpful if you need more control over the requests actually sent to the devices e.g. to optimize the query. Smaller features like an
omit
flag to fill gaps in the requests accompany the code. Most of the features and, to a large degree, the configuration scheme are borrowed from #8013. Thank you @wz2b for the nice work there!