suggested changes to protobuffer after eds mapping experience

[nimrof]

Hi,
Got some changes to protobuffer i want to discuss

1. Change index & subindex from string to integer
   Its easier to work with integers and i not sure if there is a good reason why its a string.
2. Add the remaining OD object datatypes (null,domain,deftype,defstruct)
   It makes it easier to map other standard formats
3. Add a map<string, string> to object & subobjects
   That will allow us to store non-standard key & values in a standard way and would be equivalent to xdd property that is currently used to store canopennode values.
   We could probably add some generic known key system that could allow us to treat them as integer/enum/other in the gui although they are stored at string.
   This could also be a better way to treat some of canopennode non-standard data in a generic way
4. The storage format should be repeated CanOpenDevice
   That allow us to store multiple devices in a single file if we want, its a little tricky/hacky to parse multiple files if we write just write multiple files without protobuffer support

[CANopenNode]

Hi, sorry for mu inactivity last two weeks. Here are some mu thoughts:

1. Change index & subindex from string to integer
   Its easier to work with integers and i not sure if there is a good reason why its a string.

String adds some extra work to the programmer, but it is much more readable from the json. I think, it is very important. That is the only reason, I prefer string.

2. Add the remaining OD object datatypes (null,domain,deftype,defstruct)
   It makes it easier to map other standard formats

I think, that would only bloat the proto file. Canopeneditor does not use them in any way and currently I don't see practical usage. We can still produce perfectly correct eds file without that.

3. Add a map<string, string> to object & subobjects
   That will allow us to store non-standard key & values in a standard way and would be equivalent to xdd property that is currently used to store canopennode values.
   We could probably add some generic known key system that could allow us to treat them as integer/enum/other in the gui although they are stored at string.
   This could also be a better way to treat some of canopennode non-standard data in a generic way

CANopen has great standards and I like them very much. But I like little less the standards for eds and xdd. I don't like to add complexity to the proto file just to strictly follow the (imperfect) standard. I think, it is enough, if we produce correct eds file from our data. And our data contains all necessary for that. And we could freely discuss about the shape of our data.

4. The storage format should be repeated CanOpenDevice
   That allow us to store multiple devices in a single file if we want, its a little tricky/hacky to parse multiple files if we write just write multiple files without protobuffer support

I agree with that. We could add optional node-id, name, description for each device.

Sorry for my negative opinions. I was thinking about that a lot in the past and I have some quite strong opinions. But we can always discuss and find better solutions. Also, some other users may have expectations, I'm not avare of.

[trojanobelix]

CANopen has great standards and I like them very much. But I like little less the standards for eds and xdd. I don't like to add complexity to the proto file just to strictly follow the (imperfect) standard. I think, it is enough, if we produce correct eds file from our data. And our data contains all necessary for that. And we could freely discuss about the shape of our data.

That surprises me a little ;-)
But I can cope with it.

I'm quite a fan of sticking strictly to the standard. There are often considerations behind it that I can't assess. Deviations should be well justified (e.g. obvious errors)

[nimrof]

Hi, sorry for mu inactivity last two weeks.

no problem

1. Change index & subindex from string to integer
   Its easier to work with integers and i not sure if there is a good reason why its a string.

String adds some extra work to the programmer, but it is much more readable from the json. I think, it is very important. That is the only reason, I prefer string.

Did not think about that usage and i see your point, but not sure if that case outweigh the extra work.
I am guessing the problem is that json only support desimal and canopen indexes is expected to be hex and that makes it very hard to read?

I see some problems/questions:

• We cant protobuffer enforce a format when using string, so users may expect that decimal & hex is supported and some might think that 0x is not needed for hex string.
• [if hex and dec is supported] Should we preserve the way it was written or can the editor just convert it to hex on save?
  I think we should preserve and that will cause a loot of extra work.
• If hex and dec is supported how to solve potential multiple entries with same value?
• Adding extra checks like enforcing 0x hex string would help, but its would only exist in our code and still in our code we must have extra steps all over complicating the code

2. Add the remaining OD object datatypes (null,domain,deftype,defstruct)
   It makes it easier to map other standard formats

I think, that would only bloat the proto file. Canopeneditor does not use them in any way and currently I don't see practical usage. We can still produce perfectly correct eds file without that.

I see, but we cannot perfectly convert from one format to a different one and Canopen editor can support them in the future :)
The extra bloat in the protobuffer will reduce format conversion strangeness

3. Add a map<string, string> to object & subobjects
   That will allow us to store non-standard key & values in a standard way and would be equivalent to xdd property that is currently used to store canopennode values.
   We could probably add some generic known key system that could allow us to treat them as integer/enum/other in the gui although they are stored at string.
   This could also be a better way to treat some of canopennode non-standard data in a generic way

CANopen has great standards and I like them very much. But I like little less the standards for eds and xdd. I don't like to add complexity to the proto file just to strictly follow the (imperfect) standard. I think, it is enough, if we produce correct eds file from our data. And our data contains all necessary for that. And we could freely discuss about the shape of our data.

The idea comes partly from #148 (comment)
If we want others to use our solutions then we should have some ways they can expand to add there own data like we have done with non-standard canopen-node values.
If we do so then why not move our canopen-node specific values to that system and in the process make our code cleaner.
Support in xdd (and hacky-sort of in eds) is just a bonus.

So its not just to support it some parts of the standard, but to use the parts of standard to make the protobuffer storage more generic and make it possible for others to expand.

4. The storage format should be repeated CanOpenDevice
   That allow us to store multiple devices in a single file if we want, its a little tricky/hacky to parse multiple files if we write just write multiple files without protobuffer support

I agree with that. We could add optional node-id, name, description for each device.

👍

Sorry for my negative opinions. I was thinking about that a lot in the past and I have some quite strong opinions. But we can always discuss and find better solutions. Also, some other users may have expectations, I'm not avare of.

No problem with negative or strong opinion, i am not expecting everyone to agree with me.
As long as we have a civilized tone there is no problem.

[CANopenNode]

For 1 and 2 I will try to write some pros and cons next week.

For 3 I agree with "Add a map<string, string> to object & subobjects That will allow us to store non-standard key & values in a standard way". But I would still like to keep some non standard properties (disabled, storageGroup) specified by protobuffer.

Standards must be "live", they have to develop. Maybe our efforts will add some contribution to them once in the future. I'm trying to follow their evolution: eds, xdd(too complex), xdd for canfd(simplified, but no space for custom properties). I also found some other canopen device developers to use custom simple format for device description. Nanotec doesn't offer eds or xdd at all.

[trojanobelix]

I don't know the nooks and crannies of eds and XDD in detail. But the possibilities of the CanOpenEditor to convert different formats (via export) is great. It has a high benefit for users.
The many file formats how node information is exchanged over the CANopen network topology (XDD/DCF/XDC, XDP, CODB, ...) is very confusing, especially for new CANopen users.

I am not aware of any tools that can handle the different formats so comprehensively (if you know of one: please share). That is why I have come to appreciate the CANopenEditor.

I think it would be important to retain this benefit when using the protobuffer. But this is certainly possible even if a different (read: another) data format is used internally.

Nanotec does not offer eds or xdd.XDD

I think this is a serious mistake. I consider the exchange via EDS and XDD files, as standard exchange formats, to be essential.
I have just reissued five 20-year-old IO-Boads and switched to CANopenNode. Without EDS files from the old boards, it would be difficult to find out what the boards support via CANopen.

btw: @robincornelius changed the internal main file format form proprietary XML to XDD six years ago
[https://github.com/robincornelius/issues/74]

[CANopenNode]

Nanotec does not offer eds or xdd.

I think this is a serious mistake. I consider the exchange via EDS and XDD files, as standard exchange formats, to be essential.

I agree with that. But, I think, some responsibility is also in side of the standard. I didn't find eds files on their web page, but they may be accessible by email.
My point here is, that Nanotec found simple approach to their family of CANopen drives and they didn't "complicate" with standards. Here is an example from their xml file:

	<entry index='0x1000' private='false'>
		<name>Device Type</name>
		<dataType>UNSIGNED32</dataType>
		<maxSubIndex>0</maxSubIndex>
		<objectCode>VARIABLE</objectCode>
		<description></description>
		<firmwareVersion></firmwareVersion>
		<changeLog></changeLog>
		<saveable>no</saveable>
		<subEntry subindex='0x00'>
			<name>Device Type</name>
			<dataType>UNSIGNED32</dataType>
			<access>read only</access>
			<pdoMapping></pdoMapping>
			<bitLength>32</bitLength>
			<defaultValue></defaultValue>
			<defaultValue variant='PD4-C5918M4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C6018L4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-CB59M024035-E-08'>131474</defaultValue>
			<defaultValue variant='PD4-C5918L4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C5918L2104-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C5918X4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C5918X4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C5918M4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C6018L4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-C5918L4204-E-08'>262546</defaultValue>
			<defaultValue variant='PD4-CB59M024035-E-08'>131474</defaultValue>
			<validValues></validValues>
		</subEntry>
	</entry>

I took 100 lines of python to generate json file according to our CANopen.proto specification. CANopenEditor could then generate any standard file from it.

[CANopenNode]

1. Change index & subindex from string to integer
   Its easier to work with integers and i not sure if there is a good reason why its a string.

String adds some extra work to the programmer, but it is much more readable from the json. I think, it is very important. That is the only reason, I prefer string.

Did not think about that usage and i see your point, but not sure if that case outweigh the extra work. I am guessing the problem is that json only support desimal and canopen indexes is expected to be hex and that makes it very hard to read?

I see some problems/questions:

* We cant protobuffer enforce a format when using string, so users may expect that decimal & hex is supported and some might think that 0x is not needed for hex string.

* [if hex and dec is supported] Should we preserve the way it was written or can the editor just convert it to hex on save?
  I think we should preserve and that will cause a loot of extra work.

* If hex and dec is supported how to solve potential multiple entries with same value?

* Adding extra checks like enforcing 0x hex string would help, but its would only exist in our code and still in our code we must have extra steps all over complicating the code

Currently our protobuffer gives this json output:

{
  ...
  "objects": {
    "1000": {
      "disabled": false,
      "name": "Device type",
      "alias": "",
      "description": "* bit 16-31: Additional information\n* bit 0-15: Device profile number",
      "type": "OBJECT_TYPE_VAR",
      "countLabel": "NMT",
      "storageGroup": "PERSIST_COMM",
      "flagsPDO": false,
      "subObjects": {
        "00": {
          "name": "",
          "alias": "",
          "type": "UNSIGNED32",
          "sdo": "ACCESS_SDO_RO",
          "pdo": "ACCESS_PDO_NO",
          "srdo": "ACCESS_SRDO_NO",
          "defaultValue": "0x000F0191",
          "actualValue": "",
          "lowLimit": "",
          "highLimit": "",
          "stringLengthMin": 0
        }
      }
    },
    "1001": {
...

Currently key for index is string with four hex digits and for subindex with two hex digits. Maybe prepending "0x" would be more clear. But I insist with hex numbers or json file will lose all human readability.

I think, some validation of the database would be very useful. Protobuffer itself does not offer validation except data types. As I know, application level validation is suggested. It also does not provide uint8_t or uint16_t datatype. So that makes no difference if we just use string. And string suits perfectly into json.

EDS uses [1000], xdd uses index="1000". I suggest the same: four hex digits without leading "0x" and we mention that in CANopen.proto file. CANopenEditor does not even need to convert it internally into integer, it could simply use string anywhere.

[CANopenNode]

2. Add the remaining OD object datatypes (null,domain,deftype,defstruct)
It makes it easier to map other standard formats

I think, that would only bloat the proto file. Canopeneditor does not use them in any way and currently I don't see practical usage. We can still produce perfectly correct eds file without that.

I see, but we cannot perfectly convert from one format to a different one and Canopen editor can support them in the future :) The extra bloat in the protobuffer will reduce format conversion strangeness

This is a long story. I was thinking about deftype and defstruct in the past. Short answer: deftype and defstruct are not used (totally unused) by CANopenEditor, there are only two search results from this project: this issue and one place in the code, where enum is defined. Nobody never asked for them.

deftype: If CANopenEditor use deftype, then DataType must not be specified as enum. It has to be something dynamically defined. I don't vote for that.

defstruct: I was seriously thinking about implementing defstruct into CANopenEditor. CANopen objects with the same complex data type(records), would then be forced to have the same structure, as specified by defstruct. There are pros and cons and are also some inconsistencies in the standard. I found much simpler to keep defstruct unused, data structure is specified by each object individually.

If we include null,domain,deftype,defstruct into proto definition, there will be problems explaining what they actually mean.

If we don't use null,domain,deftype,defstruct, then some entries from eds file will probably be skipped. However, when exporting eds file, we could still generate deftype and defstruct objects from internal data.

There is also some philosophy: CANopen.proto is new, we don't have to take the burden (of obsolete parts) from the standards. I suggest, we try to keep it as simple as possible. It is very easy to add some properties into proto file in the future if there will be a real reason.

[CANopenNode]

4. The storage format should be repeated CanOpenDevice
That allow us to store multiple devices in a single file if we want, its a little tricky/hacky to parse multiple files if we write just write multiple files without protobuffer support

I agree with that. We could add optional node-id, name, description for each device.

Actually there is no need for additional node-id, name, description for each device. Those information is already included inside CanOpen_DeviceCommissioning. Repeated CanOpenDevice would be just fine. Or maybe map<string, CanOpenDevice> to locate CanOpenDevice by user specified string(name).

[CANopenNode]

3. Add a map<string, string> to object & subobjects
That will allow us to store non-standard key & values in a standard way and would be equivalent to xdd property that is currently used to store canopennode values.
We could probably add some generic known key system that could allow us to treat them as integer/enum/other in the gui although they are stored at string.
This could also be a better way to treat some of canopennode non-standard data in a generic way

As I mention above, I think it is a very good idea. We could discuss later how to use individual non-standard properties.