openenergysolutions / openfmb.adapters

OpenFMB Adapters
Apache License 2.0
3 stars 1 forks source link

Modbus: map 32 bit float fields held in two 16 bit Modbus registers #45

Closed jadamcrain closed 5 years ago

jadamcrain commented 5 years ago

Unsure if the configuration allows for this or not

larrylackey commented 5 years ago

The Modbus protocol Parker inverter from last year has a Float32 in two Modbus registers as MSR. But wouldn't you know it the new microturbines just installed last week have Float32 values in two Modbus registers as LSR :-( Please see attached image from SEL manual explaining difference, and sadly yes we need both MSR and LSR depending upon the device.

image

emgre commented 5 years ago

The current implementation allows for signed and unsigned 16-bit and 32-bit, and the modulus variation for 32-bit. It does not allow IEEE 754 at the moment. I'm working on it, with a Little Endian and Big Endian variations, as described in the SEL doc.

emgre commented 5 years ago

With the PR I just open, you can now have the following mapping:

value:
  float-field-type: mapped
  source-type: holding_register  # {none, holding_register}
  register-mapping: float32  # {sint16, uint16, sint32, uint32, sint32_with_modulus, uint32_with_modulus, float32}
  lower_index: 0
  upper_index: 1
  scale: 1

It will poll the two registers and parse them as IEEE 754.

Since the lower and upper indices are specified manually, the behaviour specified in the SEL doc can easily be achieved by swapping the two indices.

larrylackey commented 5 years ago

"Since the lower and upper indices are specified manually, the behaviour specified in the SEL doc can easily be achieved by swapping the two indices.""1..0" seems un-obvious. How about "float32lsr" and "float32msr" as register-mapping options? Thoughts?

On Thursday, July 25, 2019, 12:47:43 PM MDT, Émile Grégoire <notifications@github.com> wrote:  

With the PR I just open, you can now have the following mapping: value: float-field-type: mapped source-type: holding_register # {none, holding_register} register-mapping: float32 # {sint16, uint16, sint32, uint32, sint32_with_modulus, uint32_with_modulus, float32} lower_index: 0 upper_index: 1 scale: 1

It will poll the two registers and parse them as IEEE 754.

Since the lower and upper indices are specified manually, the behaviour specified in the SEL doc can easily be achieved by swapping the two indices.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

emgre commented 5 years ago

The lower_index and upper_index mechanism was already implemented for all the 32-bit mappings. To be consistent, I would let it as is.

larrylackey commented 5 years ago

That's good that the order is already accounted for. However, it required out-of-band knowledge for someone to do that. To avoid required that magic knowledge, how about adding to the # help for Modbus numeric values msr and lsr examples?

On Thursday, July 25, 2019, 1:25:33 PM MDT, Émile Grégoire <notifications@github.com> wrote:  

The lower_index and upper_index mechanism was already implemented for all the 32-bit mappings. To be consistent, I would let it as is.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

jadamcrain commented 5 years ago

@larrylackey: The issue with adding comments is that we can only generate one of the variations. There's a broad range of choices here ( sint16, uint16, sint32, uint32, etc).

To me, this points to the same long term issue: no documentation or schema.

larrylackey commented 5 years ago

Agreed on the longer term. Just trying to get something general for now that doesn't require look up something in the Modbus library. My understanding from an earlier comment was that all the numeric values were treated the same way. How about something like:

For registers a, b, c, d... LSR is _ ; MSR is  __ lower_index b and upper_index c lower_index c and upper_index b (I don't want to make a mistake and confuse everything so just fill in the right answer)

Is this any better?Thanks

On Thursday, July 25, 2019, 2:48:17 PM MDT, Adam Crain <notifications@github.com> wrote:  

@larrylackey: The issue with adding comments is that we can only generate one of the variations. There's a broad range of choices here ( sint16, uint16, sint32, uint32, etc).

To me, this points to the same long term issue: no documentation or schema.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jadamcrain commented 5 years ago

well, we can certainly change the current names for all 32-bit types:

upper_index -> msr_index lower_index -> lsr_index

Does this add value? We're just pushing back against treating 32-bit integers differently than float for the purpose of naming the indices.

larrylackey commented 5 years ago

I agree and thought that's what Emile was saying is that the same  b to c or c to approach worked for both ints and floats.

On Friday, July 26, 2019, 12:22:08 PM MDT, Adam Crain <notifications@github.com> wrote:  

well, we can certainly change the current names for all 32-bit types:

upper_index -> msr_index lower_index -> lsr_index

Does this add value? We're just pushing back against treating 32-bit integers differently than float for the purpose of naming the indices.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.