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

A uniform way do interact with importer/exporter #122

Open
4 of 5 tasks
Tracked by #102
nimrof opened this issue Jul 16, 2024 · 9 comments
Open
4 of 5 tasks
Tracked by #102

A uniform way do interact with importer/exporter #122

nimrof opened this issue Jul 16, 2024 · 9 comments
Assignees

Comments

@nimrof
Copy link
Collaborator

nimrof commented Jul 16, 2024

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)

Sounds good ?
@CANopenNode & @trojanobelix

@nimrof nimrof mentioned this issue Jul 16, 2024
20 tasks
@trojanobelix
Copy link
Collaborator

For the strings, it sounds good to me.

@CANopenNode
Copy link
Owner

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"

@nimrof nimrof self-assigned this Jul 21, 2024
@trojanobelix
Copy link
Collaborator

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

@nimrof
Copy link
Collaborator Author

nimrof commented Jul 22, 2024

something like this?
image

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

@CANopenNode
Copy link
Owner

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
Copy link
Collaborator Author

nimrof commented Jul 22, 2024

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
Copy link
Collaborator Author

nimrof commented Jul 24, 2024

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.

public void buildmappingsfromlists(bool isCANopenNode_V4)

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?

if (!isCANopenNode_V4)
{
sub = new ODentry("compatibility entry", (ushort)slot.ConfigurationIndex, 4);
sub.datatype = DataType.UNSIGNED8;
sub.defaultvalue = "0";
sub.accesstype = EDSsharp.AccessType.rw;
config.addsubobject(0x04, sub);
}

config.parameter_name = "RPDO communication parameter";
config.prop.CO_countLabel = "RPDO";
sub = new ODentry("Highest sub-index supported", (ushort)slot.ConfigurationIndex, 0);
sub.defaultvalue = isCANopenNode_V4 ? "0x05" : "0x02";

if (isCANopenNode_V4)
{
sub = new ODentry("Event timer", (ushort)slot.ConfigurationIndex, 5);
sub.datatype = DataType.UNSIGNED16;
sub.defaultvalue = slot.eventtimer.ToString();
sub.accesstype = EDSsharp.AccessType.rw;
config.addsubobject(0x05, sub);
}

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
Copy link
Collaborator

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
Copy link
Owner

I think I made that in 32ef9eb

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.

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

No branches or pull requests

3 participants