openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.88k stars 3.59k forks source link

[modbus] Write data types should parallel read types #4148

Closed Rossko57 closed 4 years ago

Rossko57 commented 6 years ago

ENHANCEMENT REQUEST

BACKGROUND Modbus data Things are characterized by type to interpret standard 16-bit modbus registers e.g. int16, float32. A selection of types are provided. This issue concerns the 16 and 32-bit integer types, which may be of either simple or signed format.

When writing, some of the type distinctions are essentially redundant. Example, writing a 16-bit register just takes the lower 16 bits of an openHAB integer. No distinction is made between signed and unsigned numbers. This all works fine (when the given number is within the int16 or uint16 range).

Current effect is that only "int16" "int32" are valid write data types.

PROBLEM Simply, it is counter intuitive to set up a read for uint16 (unsigned) and write to the very same register with int16 (signed). Likewise int32/uint32 , int32swap/uint32swap, for paired registers.

SUGGESTED SOLUTION Allow use of u-integer data types in data Thing write parameters, to parallel read parameters. Note - this does not change functionality at all (although it could be be taken into account in any future validation of write data values).

It's a cosmetic fix, for consistency in user configuration - an ease of use improvement.

Discussion https://community.openhab.org/t/modbus-openhab2-binding-available-for-alpha-testing/27657/566

ssalonen commented 6 years ago

Turns out the uint16 & co. are supported in textual configuration, see this post https://community.openhab.org/t/modbus-openhab2-binding-available-for-alpha-testing/27657/623?u=ssalonen

However, if one has configured it textually, the Paper UI shows the write value type as "empty": dropdown has "Select a value"

Rossko57 commented 5 years ago

Does that mean some PaperUI fix is needed, or can this be closed now?

ssalonen commented 5 years ago

My previous comment was further clarification and information to the topic based on my own testing.

The Paper UI is not showing the "correct"/reasonable value (when uint16 is configured as write value type), although everything "works" as expected behind the scenes.

Currently Paper UI has the following UI options for write value type

<option value="float32">32bit floating point (float32)</option>
<option value="float32_swap">32bit floating point, 16bit words swapped (float32_swap)</option>
<option value="int32">32bit integer, as two's complement (int32)</option>
<option value="int32_swap">32bit signed integer, as two's complement but with 16bit words swapped (int32_swap)</option>
<option value="int16">16bit integer, as two's complement (int16)</option>
<option value="bit">individual bit (bit)</option>

On purpose there is no mention of "signed" or "unsigned" integers, just "two's complements".

Ideally, I think, we would like textual configuration of uint16 to show as 16bit integer, as two's complement (int16) in UI. I wonder if this possible? M

Alternative way is to introduce 3 more variants (uint32, uint32_swap, uint16) just for visual purposes. I will be soon introducing support for 64 bit integers, which would mean two more additional variants for "unsigned" entries. This has the obvious downside that there are way more entries in UI.

Rossko57 commented 5 years ago

Okay, so those hand-cranking text config files have no problem.

Those using PaperUI have no practical problem, it's just cosmetic. Those with sharp eyes may think the option they want is missing from PaperUI selection. For them, looks like it's as simple as changing the option description e.g. <option value="int16">16bit integer, as two's complement (int16 or uint16)</option> That can wait until 64 bit changes come along

The only messed up case is using PaperUI to view a config that was already created in text editing. At worst, they see a "blank" selection. I'm inclined to just accept that as it is.

ssalonen commented 4 years ago

This seems to be still valid

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-3-milestone-discussion/132715/51

openhab-bot commented 2 years ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/solax-hybrid-x1-x3-inverter-realtime-data-and-control-locally/137371/12