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

move eds import&export to its own file #104

Merged

Conversation

nimrof
Copy link
Collaborator

@nimrof nimrof commented Apr 4, 2024

Trying to move import & export code away from eds.cs

closes #105

@nimrof nimrof self-assigned this Apr 4, 2024
@nimrof
Copy link
Collaborator Author

nimrof commented Apr 4, 2024

@CANopenNode im not done, but was it something like this you thought about?
Just move all "every" write/parse function into its own file

Suggestion about the name of the new file are welcome :)

Copy link
Collaborator

@trojanobelix trojanobelix left a comment

Choose a reason for hiding this comment

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

That looks good tp me!

@CANopenNode
Copy link
Owner

Yes, that is something I thought. I hope, about 1000 lines will go out from eds.cs.

Maybe edsImportExport.cs, could be CanOpenEds.cs, similar as other CANopen*.cs files, which do the same thing.

For eds.cs I would be more ambitious. According to my ideal, it would represent a CANopen device, a core, not related to any specific importer/exporter (eds, xml, etc.), but should contain all device information for any exporter or GUI window in a clear way. But that means rearrange of the whole file and update "99+references" to many objects all around. With some smart approach and modern tools this may not be such a hard work? Or yes?

If we decide to bite into this, then should agree all three of us and then discuss and cooperate. After finishing that part application should look and behave more or less the same as now.

@trojanobelix
Copy link
Collaborator

I think that's a good idea. But I'm afraid my C# skills aren't good enough for that. :-(

@nimrof
Copy link
Collaborator Author

nimrof commented Apr 5, 2024

Yes, that is something I thought. I hope, about 1000 lines will go out from eds.cs.
👍

Maybe edsImportExport.cs, could be CanOpenEds.cs, similar as other CANopen*.cs files, which do the same thing.

Good point, i will switch before im done.

For eds.cs I would be more ambitious. According to my ideal, it would represent a CANopen device, a core, not related to any specific importer/exporter (eds, xml, etc.), but should contain all device information for any exporter or GUI window in a clear way.

Sounds good at least in theory, and i have no better idea.

But that means rearrange of the whole file and update "99+references" to many objects all around. With some smart approach and modern tools this may not be such a hard work? Or yes?

maybe

If we decide to bite into this, then should agree all three of us and then discuss and cooperate. After finishing that part application should look and behave more or less the same as now.

Yes, agree

@nimrof nimrof marked this pull request as ready for review April 6, 2024 07:12
@nimrof
Copy link
Collaborator Author

nimrof commented Apr 6, 2024

I hope, about 1000 lines will go out from eds.cs.

1136 1389 lines :)

@nimrof nimrof marked this pull request as draft April 6, 2024 07:15
@nimrof nimrof marked this pull request as ready for review April 6, 2024 08:50
@trojanobelix
Copy link
Collaborator

I can recognise the goal and I like it. I've looked at the code and it's going in a very good direction. I hope to do some more testing next week and test the changes in the real workflow. So a "go" from me for now.

Thank you!

@nimrof nimrof merged commit 8dc739c into CANopenNode:main Apr 7, 2024
4 checks passed
@nimrof nimrof deleted the move-eds-import-and-export-to-own-file branch April 7, 2024 19:20
@CANopenNode
Copy link
Owner

eds.cs seems more manageable now. Its central part is public partial class EDSsharp, which is "electronic device description in c sharp" for a specific device we work on. With new EDSsharp() new object is created, which contains all the information: FileInfo, DeviceInfo, etc. and ods = new SortedDictionary<UInt16, ODentry>();. That is our CANopen device and I would like we look this "core" file this way.

The concept is OK, but the file is too messy.

For the beginning I will write a new file (just a proposal, without references) with rewritten EDSsharp class (or name it CanOpenDevice). It will be basically a database for our CANopen device. We could discuss about the details then.

I have a question: once we have a populated CanOpenDevice class instance, can we simply serialize it into a JSON file. And later load it back. Is this a good idea?

@nimrof
Copy link
Collaborator Author

nimrof commented Apr 9, 2024

eds.cs seems more manageable now. Its central part is public partial class EDSsharp, which is "electronic device description in c sharp" for a specific device we work on. With new EDSsharp() new object is created, which contains all the information: FileInfo, DeviceInfo, etc. and ods = new SortedDictionary<UInt16, ODentry>();. That is our CANopen device and I would like we look this "core" file this way.

The concept is OK, but the file is too messy.

Agree

For the beginning I will write a new file (just a proposal, without references) with rewritten EDSsharp class (or name it CanOpenDevice). It will be basically a database for our CANopen device. We could discuss about the details then.

👍

I have a question: once we have a populated CanOpenDevice class instance, can we simply serialize it into a JSON file. And later load it back.

Yes, and its not that hard, but not sure how it works if we add/remove fields from the classes at a later time, but that is a problem with most formats.
https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/deserialization
System.Text.Json

Is this a good idea?

I am not sure yet, but not against the idea.

What we need is a format that has "native" support for everything that xdd and edi has + canopennode things?
We can use xdd and add use the xdd properties to store canopennode things, but we do not want that because ?

@CANopenNode
Copy link
Owner

JSON seems very simple to me, almost 1:1 with the class, all automatic. As from the link:

Any JSON properties that aren't represented in your class are ignored by default. Also, if any properties on the type are required but not present in the JSON payload, deserialization will fail.


What we need is a format that has "native" support for everything that xdd and edi has + canopennode things?
We can use xdd and add use the xdd properties to store canopennode things, but we do not want that because ?

xdd1_1 stores canopennode things. It is also used as default project file. But that format is very complex and CANopenEditor supports it only partly. Luckily it allows storing custom properties, but limited. JSON seems just so much easier and more useful for the project file. Besides JSON can be loaded also to python or other languages. Of course, we could still use xdd or eds for import/export, as now.

@nimrof
Copy link
Collaborator Author

nimrof commented Apr 9, 2024

okay, sounds good to me

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.

Move import & export part of eds away from eds.cs
3 participants