Model 11 MAC address broken?

andig commented 5 years ago

I'm trying to use gosunspec to work with an SMA grid inverter. Reading model 11 fails with a modbus exception when I try to read the first 6 words according to the model 11 definition:

<block len="13">
  <point id="Spd" offset="0" type="uint16" mandatory="true" units="Mbps" />
  <point id="CfgSt" offset="1" type="bitfield16" mandatory="true" >
    <symbol id="LINK">0</symbol>
    <symbol id="FULL_DUPLEX">1</symbol>
    <symbol id="AUTO_NEG1">2</symbol>
    <symbol id="AUTO_NEG2">3</symbol>
    <symbol id="AUTO_NEG3">4</symbol>
    <symbol id="RESET_REQUIRED">5</symbol>
    <symbol id="HW_FAULT">6</symbol>
  </point>
  <point id="St" offset="2" type="enum16" mandatory="true" >
    <symbol id="UNKNOWN">0</symbol>
    <symbol id="ENABLED">1</symbol>
    <symbol id="DISABLED">2</symbol>
    <symbol id="TESTING">3</symbol>
  </point>
  <point id="MAC" offset="3" type="eui48" />
  <point id="Nam" offset="7" type="string" len="4" access="rw" />
  <point id="Ctl" offset="11" type="bitfield16" access="rw" >
    <symbol id="AUTO">0</symbol>
    <symbol id="FULL_DUPLEX">1</symbol>
  </point>
  <point id="FrcSpd" offset="12" type="uint16" access="rw" units="Mbps" />
</block>

It does seem to work if I read 7 words. Is it possible that the MAC should actually start at offset 4 and a padding or else be applied before it?

altendky commented 5 years ago

If we look at the official Python implementation of a SunSpec client it looks like an eui48 field is 64 bits/4 registers long with the lowest register address being 'unused'. Note the lack of use of data[0] and data[1] in data_to_eui48() and the prefixed b'\x00\x00' in eui48_to_data(). The spreadsheet at https://sunspec.org/wp-content/uploads/2017/09/SunSpecInformationModelReference20170928.xlsx agrees that the point length is 4.

https://github.com/sunspec/pysunspec/blob/5242ea9e4c687061cc0b8de4417b1943ed8f9264/sunspec/core/util.py#L75-L86

def data_to_eui48(data):
    if sys.version_info < (3,):
        data = [ord(x) for x in data]

    value = False
    for i in data:
        if i != 0:
            value = True
            break
    if value and len(data) == 8:
        return '%02X:%02X:%02X:%02X:%02X:%02X' % (
            data[2], data[3], data[4], data[5], data[6], data[7])

https://github.com/sunspec/pysunspec/blob/5242ea9e4c687061cc0b8de4417b1943ed8f9264/sunspec/core/util.py#L153-L154

def eui48_to_data(eui48):
    return (b'\x00\x00' + base64.b16decode(eui48.replace(':', '')))

But yeah, eui48 seems to be missing entirely from https://sunspec.org/wp-content/uploads/2015/06/SunSpec-Information-Models-12041.pdf (or my searching is failing).

Additionally, this model doesn't seem to follow alignment suggestions.

Use PAD to keep 32 and 64 bit alignment As a consideration to low resource devices model designers should align all 32 and 64 bit values on even numbered offsets. A PAD register should be added at the end of the fixed length block and/or at the end of a repeating block to ensure both add up to an even number of registers. Arrange the other points as needed to place the PAD at the end of the block.

So either another 16-bit value should be moved before MAC and Nam or one that is before moved to after.

And finally, I wouldn't generally expect a Modbus exception when reading a group of registers which are a subset of another group you can read. Modbus shouldn't care if you are reading only the first three out of four registers that make up MAC. Unless maybe I've missed this as a deviation that SunSpec requires from normal Modbus.

andig commented 5 years ago

Great, in-depth answer, thank you very much! As for

I wouldn't generally expect a Modbus exception when reading a group of registers which are a subset of another group you can read. Modbus shouldn't care if you are reading only the first three out of four registers that make up MAC.

I wouldn't know but can tell you that I've seen this behaviour in 5 different SMA devices, for whatever its worth...

altendky commented 5 years ago

Open items:

[ ] Documentation for eui48 type
[ ] Achieve even alignment for Nam and MAC?
- Or will it be argued that these would be allocated on the inverter as an array of bytes and as such the alignment is not relevant?
[ ] Pad to even model length
[ ] Possibly add tests for both model length and multi-register point alignment

cfstras commented 5 years ago

It seems like the "Not Implemented"-value for eui48 is also not defined. My first guess would have been 0x000.., since that's what string uses. However, pysunspec lists ff:ff:ff:ff:ff:ff (https://github.com/sunspec/pysunspec/blob/master/sunspec/core/suns.py#L80), which is underspecified -- are the unused bytes set to 0 or to ff?

andig commented 5 years ago

I think it should only check starting at the second word as the first is ignored. Couln't immediately find it in the source though. Also see https://github.com/sunspec/pysunspec/issues/74

nielsbasjes commented 5 years ago

FYI: I have an SMA SunnyBoy 3.6 device that fills all 8 bytes with 0xFF if the MAC address is unspecified.

Can someone please post a 'real' value (i.e. all 8 bytes) when it is filled? Main question: Do real devices put a 0XFF or a 0x00 in the first two bytes if it is filled?

ghost commented 5 years ago

In terms of documentation, yes eui48 type should be documented and that is in progress.
For model 11 we cannot achieve alignment because the model is already being used by manufacturers.
Type eui48 is implemented as uint64, so the unimplemented value is 0xFFFFFFFFFFFFFFFF
As was described eui48 resides in the lower 6 bytes
It seems that SMA requires that multi-register values be read together for synchronization purposes.

sunspec / models

Model 11 MAC address broken? #41