sunspec / models

SunSpec Model Definitions
Apache License 2.0
84 stars 52 forks source link

Inconsistencies between JSON and SMDX models #237

Open andig opened 1 year ago

andig commented 1 year ago

Here are some observations I've made regarding https://github.com/sunspec/models/issues/62#issuecomment-1288095736:

nielsbasjes commented 1 year ago

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:

CC: @bobfox @shelcrow

andig commented 1 year ago

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 https://github.com/sunspec/models/issues/62, it is 2.5 years old...

nielsbasjes commented 1 year ago

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

andig commented 1 year ago

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

bobfox commented 1 year ago

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 .

andig commented 1 year ago

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 1 year ago

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.
  2. 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!).

  3. 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: https://github.com/sunspec/models/issues/240

nielsbasjes commented 1 year ago

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

About the len; I have seen https://github.com/sunspec/models/issues/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:

nielsbasjes commented 1 year ago

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.