modbus_gateway input plugin

[wz2b]

Required for all PRs:

• Signed CLA.
• Associated README.md updated.
• Has appropriate unit tests.

The motivation behind this plugin is to address the situation where there are multiple devices
behind a gateway, but the gateway can't accept many TCP connections at once. This is
a typical case (unfortunately) because many gateways on the market are made using
small microcontrollers with small RAM and limited TCP/IP stacks. Even some
expensive devices, like the PowerLogic CM4200, only allow 4 simultaneous connections.
The MBAP
protocol includes a unit identifier in each request specifically so that one TCP
connection can be shared talking to multiple devices. (Historical note: Modbus/UDP exists
as well, but is used less often).

The Modbus Gateway plugin collects Input Registers and Holding
Registers via Modbus TCP. It is similar to the "modbus" driver (and uses the same
underlying protocol implementation) but has a different configuration format suited to
communicating with Modbus/TCP devices acting as gateways. A gateway is still a modbus server,
but instead of reaching one device a gateway has many real or virtual devices attached to it.
The devices beyond the gateway are often connected via modems, radios, or multi-drop RS-485
or RS-422 buses.

Before writing this, I attempted to modify existing modbus plugin but the changes were too extensive to support multiple unit addresses in a single PDU, so I started over. Extensive explanation why this is needed in the README.md file. I plan additional enhancements, but this is good enough for many purposes as-is and since this is my first contribution I'm going to keep it simple and start here for now.

[ssoroka]

Looks good. I'm going to ping the modbus contributors to look at this as they have a lot more context than I do.

[ssoroka]

cc @garciaolais @srebhan @sensor-freak in case you're interested and have any feedback

[ssoroka]

one fix left, otherwise lgtm. Will wait a few days for community feedback.

[sensor-freak]

Good work, thank you!

But please let me comment on one point: the usage and implicit conversion of the data type.

You implicitly convert INT types to float values, even if no offset/scale is given. This might not always be desireable. As far as I can see, all INT measurements are converted to floating point values on the output (Influx or whatever). But sometimes one wants to keep the INT-property on the output. For this the original modbus plugin differentiates between INT types and FIXED types. Using INT types keps the values as integers, even when scaling/offset is applied, while the FIXED types behave similar to your implementation. (See also discussions on PR #7869, and maybe also issue #7317 and PR #7488.)

Maybe this can be kept in mind when the plugin gets extended in the future?

[srebhan]

First of all thanks for this PR @wz2b! I'm also fighting the problem with gateways accepting only a few connections but I was able to work-around these in the past. :-)
To understand the differences in logic I compared the code of your new plugin with the existing "modbus" plugin. To me it seems like the only difference is that you do not set the "slave-ID" during connect() but when gathering the registers. This being said, to me it would be better to extend the existing plugin with the @wz2b's idea. This could be done by allowing to (optionally) override the slave-id in the register definition and changing the logic to group the registers by slave-id. This way we avoid code duplication and you get features like retry-when-busy for free.

What do you think? I could help with modifying the modbus plugin and/or test the code...

[wz2b]

You implicitly convert INT types to float values, even if no offset/scale is given. This might not always be desireable

Thank you, you're right -- I tried to be clever and say: it starts out an int/uint, so if it's still an int/uint after scaling write it as such. If someone takes an int and scales it by a fraction, it's not an int any more, so I'll write it as float. My idea was to do that by looking at presence of a scaling factor other than 1. I had planned on a subsequent PR to make it smart enough to keep it an int if scale/offset are ANY int, but now reading what you wrote I think you're right i I need something like 'fixed.' I will plan for that when I add discrete and floats, along with good documentation to explain why. Thanks!

[wz2b]

This being said, to me it would be better to extend the existing plugin with the @wz2b's idea

I started out there - basically it meant making a map of device-id to registerRange, then modifying everything that touches that code to use that map - essentially wrapping the existing for loops (over registerRange) with a second loop. This means Gather(), but also Init() where it tries to find those adjacent ranges. Maybe there's a better way to do it, but what I was trying ended up making code that was really hard to read. I tried to mitigate that by refactoring and splitting files, but I felt like I was making a mess. Then I started thinking:

The way the existing plugin works, it tries to find adjacent ranges of registers and groups them into PDUs. That's probably good for most people, but I tend to want to more explicit about laying out the requests that go over the wire myself. That's why I ended up with the idea of making the definitions really map to the PDUs (rather than the fields), and secondarily have users define how those fields map to the response PDU. That was my thought, anyway ... but understand that a LOT of users polling a few registers from a single device don't care about that, so in the end I decided it would be better to not complicate what's already working.

I'd be in favor of moving some of these features (including addresses and gateways) into the original modbus plugin, I'm just not up for doing it myself at least right now. I was being chased by manslaughter-hornets in my yard and I tripped and broke my arm, so I'm in a cast and typing is rather excruciating. Maybe in October 👍

Does that make sense? Do you still think merging these concepts is a good idea? And would you be OK with letting them be separate now and working toward merging (and deprecating modbus_gateway) later, or do you see that as a reason to hold this up?

[wz2b]

FYI, the underlying modbus implementation has issues. I found this one running it overnight. I don't know that protocol implementation is maintained or not. I might look for another one, or I might fix it. It would be nice to have a more threadsafe implementation that allows (or requires) you to pass the device id with every request.

Regardless, this happened on my test system overnight, and it happened to both the modbus and modbus_gateway plugins (I am running a configuration with both in parallel, to two different gateways). So not a problem with this plugin specifically.

[srebhan]

@wz2b It makes sense and don't worry about the timing! I'm still thinking that maintaining two plugins that differ only in some details is a bit burden and I'm thus in favor of merging the two. I'll let the guys from InfluxData decide if the two plugins should co-exist or be merged in the first place.
With your ok I'll try to merge the muliple-slave-IDs per plugin idea into the existing modbus plugin. However, as I'm quite busy right now don't expect anything in the next few weeks...

Get well soon!

[wz2b]

With your ok I'll try to merge the muliple-slave-IDs per plugin idea into the existing modbus plugin. However, as I'm quite busy right now don't expect anything in the next few weeks...

Go for it. I think you know what I'm after here: it's the UnitID thing, and the omit/skips. I'm happy to collaborate, test, whatever. While you're working on that, I am probably going to fork the underlying modbus implementation just to fix a few things. I'll try to issue a PR to them but the project seems to be unmaintained for 2 yrs so I'm not hopeful.

I guess I should mention that while it wasn't a big driver I did attempt to get rid of the words 'master' and 'slave' where I could The modbus organization is moving to as well so I figured it made sense to be consistent about it. One of the other reasons that I made a new plugin is so I could change that terminology without breaking anybody's existing configuration. Maybe there's a way to tell toml to support both (more than likely you'd have to do it manually in Init())

[ssoroka]

We do appreciate being backwards compatible. If it's possible that this new plugin covers all the functionality of the old, we can deprecate the old and recommend people use this one. We're flexible here.

[garciaolais]

cc @garciaolais @srebhan @sensor-freak in case you're interested and have any feedback

Hi Everyone, I will check this request!

@wz2b 👍

[wz2b]

You implicitly convert INT types to float values, even if no offset/scale is given. This might not always be desireable.

I'm still thinking about this. The problem I'm having is that there are two different conversions. If someone specifies FLOAT32 that means read two successive registers and convert to a float using IEEE-754. In that case, it only makes sense to output it as a float.

The second case is you read one or two 16-bit registers and interpret them as one of UINT32, UINT16, etc. In this case, after scaling, you might want to output them as an int, or you might want to output them as a float. I think what you're telling me is that's what FIXED does. It seems like there is no FIXED16 and FIXED32, though ... rather it uses the # of registers to determine that.

So for my approach that would be a problem. Miimally, I would need FIXED16 and FIXED32 to tell the streaming deserializer how many registers to read.

I feel like that's an indirect way of saying this. My thinking is this would be better: to each of my field definitions, add an output format option that can force it to be something other than float, then make everything else output float by default.

I'm not sure whether or not that's a good idea. Influx stores floats as float64. I wrote something in flux recently, and my modbus auto-int-if-possible logic actually caused me grief, meaning in the flux i had to do a cast:

|> map(fn: (r) => ({ r with _value: ( float(v:r.Pac) / (r.Vpv * r.Ipv))}))

This annoyed me.

So I'm thinking about this as an alternative to FIXED16/FIXED32:

fields = [ { name: "chris", type: "UINT16", scale: 5.0, output: "INT"} ]

and if you don't specify an output, it defaults to float64 output.

Just an idea ... how does it strike you?

[sensor-freak]

You implicitly convert INT types to float values, even if no offset/scale is given. This might not always be desireable.

I'm still thinking about this. The problem I'm having is that there are two different conversions. If someone specifies FLOAT32 that means read two successive registers and convert to a float using IEEE-754. In that case, it only makes sense to output it as a float.

I agree, except that you are speaking of type name "FLOAT32-IEEE" (in the orignal modbus plugin). This is something completely different (with respect to the discussion about FIXED/UFIXED Types)

The second case is you read one or two 16-bit registers and interpret them as one of UINT32, UINT16, etc. In this case, after scaling, you might want to output them as an int, or you might want to output them as a float. I think what you're telling me is that's what FIXED does. It seems like there is no FIXED16 and FIXED32, though ... rather it uses the # of registers to determine that.

So for my approach that would be a problem. Miimally, I would need FIXED16 and FIXED32 to tell the streaming deserializer how many registers to read.

During the implementation of FIXED/UFIXED in the original modbus plugin, it turned out that the size can be implicitly determined by the byte_order field. Therefore using suffixes (16, 32, 64) seemed to be redundant to me. If you feel more compfortable with using suffixes (maybe due to technical reasons), I could live with it.

Btw.: Did you think about different byte orders within the PDU? Do you need additional configuration to support at least big-endieness and little-endieness?

I feel like that's an indirect way of saying this. My thinking is this would be better: to each of my field definitions, add an output format option that can force it to be something other than float, then make everything else output float by default.

I'm not sure whether or not that's a good idea. Influx stores floats as float64. I wrote something in flux recently, and my modbus auto-int-if-possible logic actually caused me grief, meaning in the flux i had to do a cast:

|> map(fn: (r) => ({ r with _value: ( float(v:r.Pac) / (r.Vpv * r.Ipv))}))

This annoyed me.

So I'm thinking about this as an alternative to FIXED16/FIXED32:

fields = [ { name: "chris", type: "UINT16", scale: 5.0, output: "INT"} ]

and if you don't specify an output, it defaults to float64 output.

Just an idea ... how does it strike you?

This would also make sense to me, and it indeed does look good to me. As long as there are no strict reasons to favor the one or the other way, I could go with both alternatives.

And again: Did you think about different byte orders used by the devices?

Kind regards, keep going on!

[wz2b]

Did you think about different byte orders used by the devices?

Yeah, I will add that. I'm trying to figure out if you need more than ABCD and CDAB ... 16-bit registers are always big endian the specification section 4.2. So we're talking about "word order" really, not byte order - how do you take two adjacent 16-bit registers and turn them into a 32-bit value.

I don't love ABCD but maybe that's easier for some users. I think I'll add the ability to specify that.

During the implementation of FIXED/UFIXED in the original modbus plugin, it turned out that the size can be implicitly determined by the byte_order field.

Hmm, yeah. Here, I'm using a binary.Reader to do the parsing, so I have to tell that how many bytes to read. I guess the difference is this: I want to be able to specify both an input type and an output type, separately. The output type is float64 or int64. The input type is INT16, UINT16, INT32 (BE), INT32 (LE), FLOAT32 (BE), FLOAT32 (LE). (I don't think anybody uses 16-bit floats).

size	format	order	old plugin terminology	old plugin order	output format
16	integer	-	INT16	AB
16	unsigned integer	-	UINT16	AB	int or float
32	integer	MSW, LSW	INT32	ABCD	int or float
32	integer	LSW, MSW	INT32	CDAB	int or float
32	unsigned integer	MSW, LSW	UINT32	ABCD	int or float
32	unsigned integer	LSW, MSW	UINT32	CDAB	int or float
32	IEEE 754 single precision float	MSW, LSW	FLOAT32	ABCD	usually float
32	IEEE 754 single precision float	LSW, MSW	FLOAT32	CDAB	usually float

I think the conclusions I'm coming to are:

• I want to be able to specify the input and output format independently
• I DO need a way to deal with word-ordering, up front, because trying to splice it in later will be awkward

One of my starting philosophies is I didn't want anything to be "implicit." I'm fine with "reasonable defaults." I strayed from that as I went along, but you're reminding me that's a mistake. As we debate what the sizes mean, I'm really wondering if you should be able to specify size, word order, and format separately, rather than relying on some string like UINT32-LE ...

I need to think about this some.

[wz2b]

I will fix this later today ... Sorry. I made a mistake committing too soon.

[mjf]

@wz2b How difficult would it be to add support for the Modbus RTU protocol (or perhaps all of the protocols supported by the underlying Modbus library)?

[wz2b]

@wz2b How difficult would it be to add support for the Modbus RTU protocol (or perhaps all of the protocols supported by the underlying Modbus library)?

It would not be that difficult if the underlying library supports it, but that would be a direct serial connection, not a "gateway." It could be added, though. I'm wondering why, though - do you prefer this configuration format over the older one? (That was one of my reasons for doing this).

[lampra1]

It would not be that difficult if the underlying library supports it, but that would be a direct serial connection, not a "gateway." It could be added, though. I'm wondering why, though - do you prefer this configuration format over the older one? (That was one of my reasons for doing this).

Though, as an example based on your sample configuration, I expect that it would make sense for the gateway device to have a direct serial (rs485) connection to one or more energy meters and batteries and poll at the same time multiple solar inverters using modbus tcp. Do you suggest the use of both modbus plugin and modbus gateway plugin in this case?

[ssoroka]

Is there anything left for this or are we good to merge? Looks like it needs some updates; internal.Duration should now be config.Duration

[ssoroka]

@srebhan does #9141 and/or #7941 need to get merged before this?

[wz2b]

Is there anything left for this or are we good to merge? Looks like it needs some updates; internal.Duration should now be config.Duration

I have been detached from this for a while as I had to move on to other things, but if updates are needed I can jump back in. I just need to know what needs to be done. I've been using the current version (as-is) without having updated it, and it hasn't done anything funny. I did see an earlier question about being able to control polling speed with the original modbus plugin (not this one) and adding a little more control over poll rates seems like a useful feature to add to this one. I would be interested in doing that but I think it'd be better off as a separate branch and PR. So my preference would be to close out this one before I start making any enhancements. That said, if something needs fixing now, I will fix it.

[srebhan]

@ssoroka This is a completely orthogonal approach to what I did in #9141 and will add another modbus plugin.
I have a PR (based on PR #9141) that covers this PR and also allows to fold in #7941 into the current plugin while keeping all the functionality...

[wz2b]

@ssoroka This is a completely orthogonal approach to what I did in #9141 and will add another modbus plugin.
I have a PR (based on PR #9141) that covers this PR and also allows to fold in #7941 into the current plugin while keeping all the functionality...

Thank you for doing this. I wanted to put these features in the original driver but at the time I was too afraid about breaking existing configurations, so I forked - which may have been a mistake. From our discussion it sounds like you're on a good path here, and I am on board. We can continue talking.

[srebhan]

With #9279 being merged, I guess we can close this PR. @wz2b please reopen if you disagree.

[wz2b]

With #9279 being merged, I guess we can close this PR. @wz2b please reopen if you disagree.

Agree. If there are other changes/improvements to this plugin I think they would warrant a new issue. All the changes thus far are working very well, I am using in production and keeping a careful eye on things and it's 👍

Nice work (and collaboration) on this, Sven, thank you for your effort.

[srebhan]

Thanks @wz2b for your kind words. I think we also discussed on per-request tags... If so, please take a look at (and test :-)) PR #10231.