CANopenEditor icon indicating copy to clipboard operation
CANopenEditor copied to clipboard

A uniform way do interact with importer/exporter

Open nimrof opened this issue 1 year ago • 9 comments

So what i want to do is to expose a"array" of importers and exporters from libEDS that the GUI & CLI can use to list the exporters and importers

the array will be composed of classes that looks a little like this:

  • string description ['Electronic Data Sheet (*.eds)']
  • string type ['*.eds']
  • enum{flag} [multi import/export,documentation-related, canopennode-related]
  • func {like c++lambda} that can be called with a standard set of parameters and return a expected output type

So this is currently the import/export functions

Importers looks ready to use right now (ignoring experimental protobuffer)

List<EDSsharp> CanOpenXDD.readMultiXML(string file)
List<EDSsharp> CanOpenXDD_1_1.ReadMultiXML(string file)

EDSsharp CanOpenXDD.readXML(string file)
EDSsharp CanOpenXDD_1_1.readXML(string file)
EDSsharp CanOpenXDD_1_1.ReadXML(string file)
EDSsharp CanOpenXDD_1_1.ReadProtobuf(string file, bool json)

The problem with the exporters is the there are a lot of different parameters Exporters

void DocumentationGen.genhtmldoc(string filepath, EDSsharp eds)
void DocumentationGen.genmddoc(string filepath, EDSsharp eds, string gitVersion)
void CanOpenXDD.writeXML(string file, EDSsharp eds)
void CanOpenXDD_1_1.WriteXML(string file, EDSsharp eds, string gitVersion, bool deviceCommissioning, bool stripped)
void CanOpenXDD_1_1.WriteProtobuf(string file, EDSsharp eds, bool json)

void CanOpenXDD.writeMultiXML(string file, List<EDSsharp> edss)
void CanOpenXDD_1_1.WriteMultiXML(string file, List<EDSsharp> edss, string gitVersion, bool deviceCommissioning)

So can we reduce the number of different parameters?

  • string filepath,folderpath,file suggestion: Use only filepath, folderpath can we get from filepath (like now)
  • string gitVersion suggestion: move gitversion from gui to libEDS so it does not have to be sent with parameter.
  • string odname suggestion: Remove it, it is just Path.GetFileNameWithoutExtension(FileName) so it can be moved into the function
  • bool json, deviceCommissioning,stripped suggestion: expose multiple exporter with it false/true (almost like now)
  • InfoSection.Filetype ft (File_EDS, File_DCF) suggestion: expose multiple exporter with it set to eds / dcf (like now)
  • [x] #124
  • [x] remove odname parameter in CanOpenNode exporters
  • [x] #126
  • [ ] expose importers from libEDS
  • [x] expose exporters from libEDS

Sounds good ? @CANopenNode & @trojanobelix

nimrof avatar Jul 16 '24 18:07 nimrof

For the strings, it sounds good to me.

trojanobelix avatar Jul 17 '24 09:07 trojanobelix

Yes, It sounds good to me. For CANopenNode exporters I suggest to add exporters for v1.3(legacy) and v4.0 separately. And remove "ExporterFactory" (and remove exporter selection in preferences.)

exporter selection in preferences is used also in "DeviceODView.cs", which was introduced by me with version v4. But I don't like it - it slightly changes user interface according to canopennode version selection. For example, if v4 exporter is selected, exotic datatypes like UNSIGNED24 are not used, pdo and sdo access are handled differently.

Or maybe we can keep selection in preferences, bit rename it from "Selected exporter" to something else and use that preference only in "DeviceODView.cs"

CANopenNode avatar Jul 19 '24 16:07 CANopenNode

I don't like the exporter selection in preferences either. It would be nice if there was a more elegant solution.

trojanobelix avatar Jul 22 '24 06:07 trojanobelix

something like this? image

or is it possible/better to make a universal c/h file?

nimrof avatar Jul 22 '24 07:07 nimrof

something like this? image

Yes, this is OK. But you can also move CANopenNode exporter near other exporters into "Export..." Whatever you prefer.

or is it possible/better to make a universal c/h file?

I don't like that, files are too different.

CANopenNode avatar Jul 22 '24 10:07 CANopenNode

something like this? image

Yes, this is OK. But you can also move CANopenNode exporter near other exporters into "Export..." Whatever you prefer.

Okay, then it will be something like the image AND into export That would not alienate users (we can remove the CanOpenNode export) later

or is it possible/better to make a universal c/h file?

I don't like that, files are too different.

ok, I understand

nimrof avatar Jul 22 '24 13:07 nimrof

Hi @CANopenNode & @trojanobelix

Starting to figure out why CanOpenNode version is a config thing

This function is called almost every time something in the OD changes. https://github.com/CANopenNode/CANopenEditor/blob/40c02b38c8f4320bd3da0fdf1959c4c7f4376ff5/libEDSsharp/PDOHelper.cs#L375-L375 It looks like its only affecting the pdo communication index(s) and removing/adding subindexes based on what is supported by different CanOpenNode versions...does that make sense?

https://github.com/CANopenNode/CANopenEditor/blob/40c02b38c8f4320bd3da0fdf1959c4c7f4376ff5/libEDSsharp/PDOHelper.cs#L427-L434

https://github.com/CANopenNode/CANopenEditor/blob/40c02b38c8f4320bd3da0fdf1959c4c7f4376ff5/libEDSsharp/PDOHelper.cs#L451-L455

https://github.com/CANopenNode/CANopenEditor/blob/40c02b38c8f4320bd3da0fdf1959c4c7f4376ff5/libEDSsharp/PDOHelper.cs#L474-L481

So i suggest to change/fix it in the different canopennode exporters and generate a warning if something is changed. Sounds good or?

Not sure if when the changes are done, should it be fixed in the "EDS" structure (permanent changes is done to the "eds" file) or just fix in on export? I think it would be nice to fix in permanently as long as it is warned about

nimrof avatar Jul 24 '24 18:07 nimrof

Correct, in buildmappingsfromliststhe all the RX /TX communication objects and the mapping are created from scratch in the OD (based on the PDO list)

The differences between the CANopeNode V1.3/V4 versions are taken into account.

I agree with you that it should be a matter for the exporter to take the differences into account. I can't think of any reason why this shouldn't work.

trojanobelix avatar Jul 25 '24 07:07 trojanobelix

I think I made that in https://github.com/CANopenNode/CANopenEditor/commit/32ef9ebae8ecb54263f87409744a70c4d603a64c

CANopenNode v1.3 (legacy) requires exact structure shape for PDO communication parameter, both RPDO and TPDO (and also for PDO mapping parameters). So that exporter should be fixed. It should add "compatibility entry" automatically, adapt "Highest sub-index supported" for RPDO, exclude "Event timer" for RPDO.

CANopenNode V4 don't have such requirements, it is not strict about PDO structure shape, it should already add warnings when necessary.

I suggest removing "isCANopenNode_V4" condition and make PDOhelper work as for CANopenNode V4 and add adaptions into legacy exporter.

Here are OD requirements for V1.3: CO_OD.h and CO_PDO.h. Structures must match.

CANopenNode avatar Jul 26 '24 08:07 CANopenNode