Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change RevisionNumber to string and implemented relative TextBox #113

Closed
wants to merge 4 commits into from

Conversation

temi54c1l8
Copy link

…evice Info

@nimrof nimrof self-requested a review May 30, 2024 10:45
Copy link
Collaborator

@nimrof nimrof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,
Thanks for the pull request.

Adding extra elements to the xsd would make it fail its validation so that is not the way, but where should the extra values be?

Do any of you know @CANopenNode @trojanobelix?
The xsd has been removed from CiA for some reason 😑

@@ -679,7 +679,7 @@ public partial class DeviceInfo : InfoSection
/// product revision number according to identity object sub-index 03h (Unsigned32)
/// </summary>
[EdsExport]
public UInt32 RevisionNumber;
public string RevisionNumber="";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you want to change this into string.
Is it done to avoid converting the (hopefully) number in the textbox into a int?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RevisionNumber is like ProductNumber, I used string because it can be represented as both decimal and hexadecimal

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool always uses the string type for this data, the RevisionNumber (must have the major equal to OD 1018 subindex 3) was always set by default and could not be changed from the graphical interface

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RevisionNumber is like ProductNumber, I used string because it can be represented as both decimal and hexadecimal

I see, but how can we make sure it only contains a valid number and not a string?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool does not check this in any object, you can try for example with the ProductID, DeviceType, etc...
I changed the RevisionNumber to a string to make it conform to the rest of the existing code.

To solve your question, you should create a universal function that does this check and attach it to all objects, but it requires a major change (but this was not my goal)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to stay uint32 in database. GUI should be able to represent it as both decimal and hexadecimal.
However, ProductNumber should also be uint32...

See also our new ideas for those issues in #115

@CANopenNode
Copy link
Owner

The xsd has been removed from CiA for some reason 😑

@nimrof, what do you mean by that?

@nimrof
Copy link
Collaborator

nimrof commented Jun 3, 2024

@CANopenNode
Copy link
Owner

Interesting. I think, http://www.canopen.org/xml/1.1 is just a namespace for xml schema, actual schema does not need to be there.
However, there are still technical documents and schemas are there. Also for new CAN FD standard, there is only a standard for XDD (CiA1311) and not for EDS.

@CANopenNode
Copy link
Owner

FileRevision is part of EDS specification and is type of UNSIGNED8. xml specification only has fileVersion, both 1.0 and 1.1.

libEDSsharp/CanOpenXSD_1_1.cs is auto generated from official schema. We can not add non standard elements here.

@nimrof
Copy link
Collaborator

nimrof commented Jun 3, 2024

Interesting. I think, http://www.canopen.org/xml/1.1 is just a namespace for xml schema, actual schema does not need to be there.

Not sure what the xml standards says, but in this case the files used to be there.
https://web.archive.org/web/20240207232553/https://www.can-cia.org/xml/1.0/

However, there are still technical documents and schemas are there. Also for new CAN FD standard, there is only a standard for XDD (CiA1311) and not for EDS.

Yes, but it is currently for paying members only, so out of reach for me at the moment😶

@CANopenNode
Copy link
Owner

I'm a member, if you need some information I can look into the standards.

@CANopenNode
Copy link
Owner

RevisionNumber could be added into XDD into one of the existing properties under DeviceIdentity class somehow. libEDSsharp/CanOpenXDD_1_1.cs could be edited for that.

@trojanobelix
Copy link
Collaborator

I do not have access to the CiA DS311. But in 306 (EDS) and 301 RevisionNumber and ProductNuber are unsigned32. So I assume in DS311 it would be unsigned32, too.

(cherry picked from commit 5d94526)
(cherry picked from commit d406186)
@temi54c1l8
Copy link
Author

temi54c1l8 commented Jun 6, 2024

I changed the RevisionNumber back to UInt32 as requested, I set the display to hexadecimal to better differentiate the major and minor revision.
I also implemented the OrderCode in string as indicated in the CiA 306 1.4.0 specification.

@CANopenNode
Copy link
Owner

We can not add additional fields into XSD file, because it is not according to the standard. You can put new fields into the standard fields somehow.

device_identity_XDD

@temi54c1l8
Copy link
Author

RevisionNumber and OrderCode are in the [DeviceInfo] section.
I am not an expert in the graphics part, I close this pull request

@temi54c1l8 temi54c1l8 closed this Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants