sunspec/models

Inconsistencies between JSON and SMDX models

Opened this issue · 9 comments

andig commented

Here are some observations I've made regarding #62 (comment):

  • many (not all) fields no longer have Access: R defined. Should we assume that R is the default?
  • r/rw has become R/RW
  • len=1/2/4 for standard types is not defined. Only sometimes it is. Should we assume that len should only be defined on strings? Then it should be removed everywhere else!
  • the usage of model type/name is inconsistent, both in the old as well as in the new models
  • ID and L fields have been added to the blocks- but not everywhere?
nielsbasjes commented

What I see here is that what should be the same definitions is stored and maintained twice (as json and as xml).
The way I look at this project it should be restructured to 1 single source format and then have code generate all other formats during the build/release.
So a release will have all formats and the source will have only one.

If I can find some time I'm willing to have a look at trying to set that up.

Key questions here:

  • What is the primary format? I see more structural elements in the XML format so I would expect that to be the source.
  • Are you guys ok with having this code in Java where most of my experience lies and which is well automatable in terms of building releases and thus generating all other desired formats.

CC: @bobfox @shelcrow

  • 👍1
andig commented

The other problem is that long-standing issues- in any format- are simply not fixed. The xml is outdated and the json is inconsistent and both don't match. Community feedback is not turned into corrective actions. See #62, it is 2.5 years old...

nielsbasjes commented

To me this starts to feel like an unmaintained standard the entire solar industry is based upon. Not a nice feeling.

  • 👍1
andig commented

...and that's exactly what you notice when working with various vendor implementations (Kostal, anyone?)...

bobfox commented

Sorry for the frustration. I think there are a couple of high level points I'll make now and then come back with more specific details shortly after some review.

A point that I guess is not made explicitly enough is that the XML encoding for model definitions is deprecated. The existing definitions will always be published and available but ideally they will never be updated again. The XML modeling does not support multiple repeating groups which is used in many of the 7xx models. It is not possible to model some of 7xx models with the XML encoding. The intention is to use JSON for all models moving forward.

Once a model definition is complete, it is frozen. Exceptions are made for editorial fixes but any change must be functionally compatible with the existing model. The SunSpec model definitions are produced using the SunSpec Dashboard tool for consistency.

The last modeling push involved extending the modeling capabilities and documenting the modeling requirements. At that time all of the XML based models were moved to JSON and the 7xx models were created. A fair number of people participated in the process and many corrections were made in that effort (see the change log in the model reference spreadsheet). We are currently reviewing whether any of the non 7xx models contain inconsistencies in the JSON version.

There are a few corrections needed to the modeling specification. We are reviewing those as well and will report back with specifics as they relate to the issued raised here.

We are reviewing the all of the relevant elements and will come back with more detail shortly to address the issues you have raised
.

  • 👍1
  • 👀1
andig commented

Once a model definition is complete, it is frozen. Exceptions are made for editorial fixes but any change must be functionally compatible with the existing model.

It would really help the community if feedback was given as to which of the identified issues can't be fixed (being frozen) and which ones are addressable. It is quite frustrating to re-discover 2+ year old issues :(

nielsbasjes commented

I understand your frustration, sorry about that. I do not want to frustrate people, I want a good base to build my code upon. Both the Json and XML are a tad too unspecified in some cases, the json even has a few things that are simply wrong.

I'm fully fine with you having chosen to freeze the XML and continuing with the Json files. Having only 1 format to maintain is easier.

  1. The Json has the Model ID (ID) and Model Length (L) fields defined as part of the models. SunSpec modbus is essentially a chain of models.
  • 2 registers that are a header of the chain 'SunS'
  • Followed by repeated
    • 1 register: the model ID (or 0xFFFF/0x0000 as a terminator)
    • 1 register: number of registers of the content
    • N registers: the content
      You can only know the size and interpretation of the content AFTER the model ID and size have been read; so these first 2 registers cannot be part of the model definition as is present in the JSon definition. This is fundamentally wrong in the Json variant.

  1. The current XML definition (although frozen, it IS used) has a few places where is leans upon defaults. For something like access and mandatory it is not so much of an issue, but for len an unspecified lookuptable is needed to figure this out on a per type basis. For many types it is reasonable to assume the number of registers based upon the naming. A bitfield32 field is 2 (16 bit) registers and a float64 is 4 registers. But types like ipaddr, sf and pad a less clear. The type eui48 is even confusing, you would expect 3 registers (3*16=48) but in reality it is 4 (which is NOT specified in model 11!).

  2. The actual encoding of characters in string remains unspecified, I currently assume ASCII and not UTF-8 or something else.

The last few days I have been making some experimental software to spot the differences between json and xml.
With that I have found quite a few descriptions/labels/notes missing in the Json that are present in the XML; all not very important.
Yet I also found a true bug in the json this way that is most likely created during the migration from XML to json: #240

nielsbasjes commented

So I put up 2 pull requests for discussion/consideration (still have to sign CLA).

  • 2 genuine bugs in the current json spec I found while doing these xml cleanups: #241

  • My proposed way of cleaning up/standardizing the XML: #242
    Essentially: Remove useless default attributes that are defined in the schema as the default. Add a len everywhere because there is no explicit definition available and this copies what is done in the json variant. And because of the way I did it all attributes are in the same ordering everywhere.

About the len; I have seen #64 and looking at the json spec I see that there the len has been added everywhere. So adding it here seems like just making it 'the same'.

If you guys say this should not be added (and thus only a len for strings) then a table with all types and sizes must be made available. I checked https://sunspec.org/wp-content/uploads/2015/06/SunSpec-Information-Models-12041.pdf and most are defined but types like count and eui48 are not.

Note that I see 2 things I would change in the json representation of the spec:

  • The ID and L are part of the list definition of sunspec, not part of the models. So I would remove the ID and L from every model (i.e. make it the same as what the XML did, which is correct).
  • Add the explicit offset within a model to each point in the spec to simply make the json spec more robust (which would have avoided the bug I found in the json).
nielsbasjes commented

An additional thing I just noticed: The groups (both fixed and repeating) in the json representation of the spec do not have the size (number of registers) of each explicitly specified for each group. I think it should have that.