search

CANopenNode / CANopenEditor

CANopen Object Dictionary Editor
GNU General Public License v3.0
132 stars 62 forks source link

A uniform way do interact with importer/exporter #122

Open nimrof opened 3 months ago

nimrof commented 3 months ago

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:

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?

Sounds good ? @CANopenNode & @trojanobelix

trojanobelix commented 3 months ago

For the strings, it sounds good to me.

CANopenNode commented 3 months ago

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"

trojanobelix commented 2 months ago

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

nimrof commented 2 months ago

something like this? image

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

CANopenNode commented 2 months ago

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.

nimrof commented 2 months ago

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 commented 2 months ago

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

trojanobelix commented 2 months ago

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.

CANopenNode commented 2 months ago

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.