Revision number #196

GRoussos · 2025-11-03T10:57:50Z

This addresses the following issues:

Making RevisionNumber (and OrderCode) EDS fields editable in the GUI - DONE
Storing all numeric id fields (eg. Vendor ID, Revision Number) in numeric variables and correctly handle decimal / Hex conversion for the GUI textboxes - DONE

The following topics will be implemented in a separate MR:

Linking EDS DeviceInfo / XDD DeviceIdentity fileds to the respective object values (eg. 0x1018, 0x1008-A) - Deferred to a separate MR
Adding some CLI option to EDSSharp way to update the identity fields so that we can use a script to update the XDD/EDS files programmatically. (this might require its own branch) - Deferred to a separate MR

Initial work done by @temi54c1l8 in #113
Resolves #154 and #155 (partially)

…evice Info

(cherry picked from commit 5d94526)

(cherry picked from commit d406186)

GRoussos · 2025-11-03T15:31:11Z

I've stumbled on an issue related to EDS<->XDD conversion. CANOpenEditor seems to use an internal representation of the device profile based on the EDS format (EDSsharp class). When trying to add support for the FileRevision field of the EDS file, I noticed that this is not specified in the XDD format. Instead, if my understanding is correct, the XDD format includes just a "fileVersion" string field with "Vendor specific version of the file", i.e. no fixed format/pattern. So oconverting between that and the FilerVersion/FileRevision Unsigned8 fields of the EDS specification is not trivial and may not even be possible. We could try to use a FileVersion.FileRevision format for the XDD fileVersion. However, my first concern is how to store internal the fileVersion read from a XDD file WITHOUT CONVERTING IT so that an arbitary vendor string can be preserved when saving the XDD again. Is it OK to add / remove fields in the EDSsharp class in a way that breaks 1:1 relation with EDS fields? My preference would be to have a fileVersion string in EDSsharp class and only attempts to convert to/from numeric FileVersion / FileRevision fileds when reading or writing a EDS file. Is that OK with the project's style? @CANopenNode

GRoussos · 2025-11-04T16:20:49Z

I've stumbled on an issue related to EDS<->XDD conversion. CANOpenEditor seems to use an internal representation of the device profile based on the EDS format (EDSsharp class). When trying to add support for the FileRevision field of the EDS file, I noticed that this is not specified in the XDD format. Instead, if my understanding is correct, the XDD format includes just a "fileVersion" string field with "Vendor specific version of the file", i.e. no fixed format/pattern. So oconverting between that and the FilerVersion/FileRevision Unsigned8 fields of the EDS specification is not trivial and may not even be possible. We could try to use a FileVersion.FileRevision format for the XDD fileVersion. However, my first concern is how to store internal the fileVersion read from a XDD file WITHOUT CONVERTING IT so that an arbitary vendor string can be preserved when saving the XDD again. Is it OK to add / remove fields in the EDSsharp class in a way that breaks 1:1 relation with EDS fields? My preference would be to have a fileVersion string in EDSsharp class and only attempts to convert to/from numeric FileVersion / FileRevision fileds when reading or writing a EDS file. Is that OK with the project's style? @CANopenNode

I have implemented my suggestion for now, i.e. the file version is read from XDD and stored internally as string and only attempted to be converted to FileVersion.FileRevision when writing an EDS file

GRoussos · 2025-11-04T16:56:26Z

The first 2 goals of the MR are done - pending testing / review from someone else too. For the other 2 goals I have decided to implement them separately, after aligning with the project's core members.
MR is ready for review.
@CANopenNode do I need to do anything to get the review started?

reza0310 · 2025-11-05T15:11:13Z

The first 2 goals of the MR are done - pending testing / review from someone else too. For the other 2 goals I have decided to implement them separately, after aligning with the project's core members. MR is ready for review. @CANopenNode do I need to do anything to get the review started?

Review will start as soon as a reviewer is available without needing to do anything no worries (give it a week).

nimrof

Just a partial review, there is a loot to check and a few questions

nimrof · 2025-11-05T07:26:37Z

libEDSsharp/CanOpenXDD.cs

                                            break;
                                        case "FileRevision":
-                                            eds.fi.FileVersion = keyvalue[1];
+                                            byte.TryParse(keyvalue[1], out eds.fi.FileVersion);


silently ignoring failed parsing

Indeed, I thought about this but saw that TryParse() is already used in many places without checking the return value. I agree handling the error is better, I have updated the code to do that.

nimrof · 2025-11-05T07:26:43Z

libEDSsharp/CanOpenXDD.cs

+                                            byte.TryParse(keyvalue[1], out eds.fi.FileVersion);
+                                            break;
+                                        case "RevisionNumber":
+                                            byte.TryParse(keyvalue[1], out eds.fi.FileRevision);                                            


silently ignoring failed parsing

I have updated the code to handle parsing failure.

nimrof · 2025-11-05T07:33:42Z

libEDSsharp/CanOpenXDD.cs

+                                        case "RevisionNumber":
+                                            byte.TryParse(keyvalue[1], out eds.fi.FileRevision);                                            
                                            break;
-                                        case "RevisionNum":


why remove this?

This was replaced with "RevisionNumber" by mistake, at the time I did not realise that the vendorTextDescription is a separate way to set the device identity fields.

nimrof · 2025-11-05T09:22:53Z

libEDSsharp/CanOpenNodeExporter.cs

+            file.WriteLine(string.Format("      VendorNumber:   {0}", eds.di.VendorNumber.ToHexString()));
            file.WriteLine(string.Format("      ProductName:    {0}", eds.di.ProductName));
-            file.WriteLine(string.Format("      ProductNumber:  {0}", eds.di.ProductNumber));
+            file.WriteLine(string.Format("      ProductNumber:  {0}", eds.di.ProductNumber.ToHexString()));


why change to hex?

Vendor ID & Product ID are usually written in Hex form

nimrof · 2025-11-08T14:47:08Z

libEDSsharp/CanOpenEDS.cs

 using System.Globalization;
 using System.IO;
 using System.Reflection;
+using System.Security.Cryptography;


added by mistake?

Good catch, I think I added when testing something but forgot to remove it afterwards.

nimrof · 2025-11-08T15:16:57Z

EDSEditorGUI/DeviceInfoView.cs

                eds.di.VendorName = textBox_vendorname.Text;
-                eds.di.VendorNumber = textBox_vendornumber.Text;
+                eds.di.VendorNumber = EDSsharp.U32Parse(textBox_vendornumber.Text);
+                eds.di.RevisionNumber = EDSsharp.U32Parse(textBox_revisionnumber.Text);


just curious, what is the reason that some are hex and some are not?
For instance i would imagine revision would be non-hex.

The reason is so that we don't have to use one format and potentially make the code incompatible with some files. I have not found clear info about the format of each field so I opted to make the code more flexible.
The revision number actually consists of 2 16-bit values, see definition of 0x1018.3 in CiA 301. So having it in Hex is more natural to easily split the 2 values.

nimrof · 2025-11-08T15:20:52Z

libEDSsharp/CanOpenEDS.cs

+            }
+            else
+            {
+                return System.Convert.ToUInt32(str);


octal is not supported, it should have leading 0 without x after

True, though I think octal is not supported in general in the project. In any case, I've updated the function to also support Octal format.

CANopenNode · 2025-11-19T09:08:37Z

There is quite a lot of changes. I didn't go into the details. However, changing CanOpen.proto is always very delicate. Vendor ID and others are already in object 1018, which is mandatory anyway. Core database should not become inconsistent.

CANopenNode · 2025-11-19T09:23:00Z

In the current state of the program it is hard to add "simple things". My current goal is more ambitious, as described here: #190 (comment)

GRoussos · 2025-11-19T12:27:08Z

There is quite a lot of changes. I didn't go into the details. However, changing CanOpen.proto is always very delicate. Vendor ID and others are already in object 1018, which is mandatory anyway. Core database should not become inconsistent.

Indeed I needed to update quite a few different parts of the code for this to work, but I believe it's in good shape now. We have already been using the updated version in our project.
I had missed filling in the mapping for the added fields in CanOpen.proto, I've fixed that. I believe the fields I've added are specified for EDS and XDD files, and although they are replicated in 0x1018, some existing fields are already in the DeviceInfo / DeviceIdentity sections so I don't see a big problem. Automatically linking the DeviceInfo fileds with 0x1018 is a good next step but not required for this update to work.

GRoussos · 2025-11-19T12:29:38Z

In the current state of the program it is hard to add "simple things". My current goal is more ambitious, as described here: #190 (comment)

The code may not be perfect but it works and this updates also seems to work, i.e. the new feature is operational and nothing seems broken.

CANopenNode · 2025-11-19T12:48:45Z

I had missed filling in the mapping for the added fields in CanOpen.proto, I've fixed that. I believe the fields I've added are specified for EDS and XDD files, and although they are replicated in 0x1018, some existing fields are already in the DeviceInfo / DeviceIdentity sections so I don't see a big problem.

I don't agree changing CanOpen.proto file at this time. Methods can be used to extract information from 1018 object. Standards should be complied by the exporters, but core database must not be messy. Information must not duplicate.

I didn't check other parts of this PR.

GRoussos · 2025-11-19T13:11:36Z

I had missed filling in the mapping for the added fields in CanOpen.proto, I've fixed that. I believe the fields I've added are specified for EDS and XDD files, and although they are replicated in 0x1018, some existing fields are already in the DeviceInfo / DeviceIdentity sections so I don't see a big problem.

I don't agree changing CanOpen.proto file at this time. Methods can be used to extract information from 1018 object. Standards should be complied by the exporters, but core database must not be messy. Information must not duplicate.

I didn't check other parts of this PR.

Although I don't think it's really messy, I can revert the CanOpen.proto changes, as they are not really needed for the feature to work - I had not mapped the new fields until my latest update a few minutes before. Still, It makes more sense to me to use the DeviceInfo / DeviceIdentity structure as the ground truth for the 0x1018 fields and not vice versa.
Keep in mind, it's not only the exporters but the importers that we need to think about. The identity fields are duplicated in EDS / XDD files anyway and the importers need to handle that, including the case where 0x1018 does not agree with the DeviceIdentity section.
I've reverted CanOpen.proto without affecting the feature

CANopenNode · 2025-11-19T13:32:05Z

The identity fields are duplicated in EDS / XDD files anyway and the importers need to handle that, including the case where 0x1018 does not agree with the DeviceIdentity section.

Better to handle imported messy eds file than to export it by default.

GRoussos · 2025-11-20T08:04:06Z

The identity fields are duplicated in EDS / XDD files anyway and the importers need to handle that, including the case where 0x1018 does not agree with the DeviceIdentity section.

Better to handle imported messy eds file than to export it by default.

OK, but this was an existing problem for other fields (ProductID, VendorID), I did not introduce it here. I only added the revision number in the same way. I have noted in the description of this PR that linking the DeviceInfo / DeviceIdentity fields with the respective object values is the next step, which will resolve the mess you refer to. But it seems too much to do everything in a single PR, you already said the changes here are a lot.
Nonetheless, what do you think about using the device identity as the source for 0x1018 and not the other way around? And in the GUI we could make it so that if you change one the other also changes (with a user notification possibly). And if we import a file where 0x1018 does not match the Device Identity we use the Device Identity values and warn the user about it.
Is that OK?

CANopenNode · 2025-11-20T11:39:57Z

It seems, we have a different taste for some, according to my opinion, minor thing. It is just a taste.

As I said, I have serious plans to work on CANopenEditor. First step will be a core, which will handle a protobuffer based database. Probably there will be a method, which will get/set device information data. And that method will be responsible, how it will access specific field. At this point details doesn't really matter. I'm just very conservative about CanOpen.proto file.

GRoussos · 2025-11-20T13:36:38Z

It seems, we have a different taste for some, according to my opinion, minor thing. It is just a taste.

As I said, I have serious plans to work on CANopenEditor. First step will be a core, which will handle a protobuffer based database. Probably there will be a method, which will get/set device information data. And that method will be responsible, how it will access specific field. At this point details doesn't really matter. I'm just very conservative about CanOpen.proto file.

Maybe I don't have the same view of the project's current state. I find it a very useful tool but at the same time it has some limitations which I am interested to work on. As a first step I want to add the "small" (from the user's PoV) feature of including the RevisionNumber, and I think I have achieved that without making the code quality worse, it's just that the overall organisation and "quality" of the project could be improved from before I started working on it - and that's OK for a non-profit open-source project.

Yesterday I reverted the changes in CanOpen.proto without affecting the feature's operation, so there's no reason to worry about that now.

temi54c1l8 and others added 5 commits November 3, 2025 12:23

Change RevisionNumber to string and implemented relative TextBox on D…

e4bf2ff

…evice Info

Save revisionNumber on XDD file

6f93f4e

Implemented OrderCode

ec3b63a

(cherry picked from commit 5d94526)

Change RevisionNumber to UInt32

b4f6e3c

(cherry picked from commit d406186)

Working on FileVersion & FileRevision

0e0c08b

GRoussos force-pushed the revisionNumber branch from 1f9af8d to 0e0c08b Compare November 3, 2025 15:19

Giannis Roussos added 2 commits November 3, 2025 18:00

Working on file version/revision

586e2a1

FileVersion and FileRevision read correctly from XDD

1bccfda

GRoussos force-pushed the revisionNumber branch from df81554 to 1bccfda Compare November 3, 2025 21:29

Giannis Roussos added 3 commits November 3, 2025 23:38

Restored XSD

1680130

RevisionNumber and OrderCode set in XDD/XSD

0ee3492

Updated Hex string format

37a3686

GRoussos force-pushed the revisionNumber branch from c599c9e to 37a3686 Compare November 4, 2025 13:36

Updated identity output

d583899

GRoussos closed this Nov 4, 2025

GRoussos reopened this Nov 4, 2025

GRoussos marked this pull request as ready for review November 4, 2025 16:54

nimrof reviewed Nov 8, 2025

View reviewed changes

Giannis Roussos added 3 commits November 10, 2025 13:29

Added Octal support, fixed parsing error handling

4fa871f

Improved GetEDSFileInfo()

a8d9ec9

Reverted parsing of vendorTextDescription

ab69d07

Fixed mapping for Protobuffer

3e69fe1

Reverted CanOpen.proto changes

bf1a956

Revision number #196

Are you sure you want to change the base?

Revision number #196

Uh oh!

Conversation

GRoussos commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GRoussos commented Nov 3, 2025

Uh oh!

GRoussos commented Nov 4, 2025

Uh oh!

GRoussos commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

reza0310 commented Nov 5, 2025

Uh oh!

nimrof left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GRoussos Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GRoussos Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GRoussos Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CANopenNode commented Nov 19, 2025

Uh oh!

CANopenNode commented Nov 19, 2025

Uh oh!

GRoussos commented Nov 19, 2025

Uh oh!

GRoussos commented Nov 19, 2025

Uh oh!

CANopenNode commented Nov 19, 2025

Uh oh!

GRoussos commented Nov 19, 2025

Uh oh!

CANopenNode commented Nov 19, 2025

Uh oh!

GRoussos commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CANopenNode commented Nov 20, 2025

Uh oh!

GRoussos commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

GRoussos commented Nov 3, 2025 •

edited

Loading

GRoussos commented Nov 4, 2025 •

edited

Loading

GRoussos Nov 10, 2025 •

edited

Loading

GRoussos Nov 10, 2025 •

edited

Loading

GRoussos Nov 10, 2025 •

edited

Loading

GRoussos commented Nov 20, 2025 •

edited

Loading

GRoussos commented Nov 20, 2025 •

edited

Loading