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

Unified file exporter interface #131

Merged
merged 8 commits into from
Sep 25, 2024
Merged

Conversation

nimrof
Copy link
Collaborator

@nimrof nimrof commented Aug 11, 2024

Hi all.
I ended up delaying changes to the GUI, it was more complicated as the canopennode version has a loot of connection to the gui, and #130 made me focus on the cli part

A little explaining is needed on the changes
Please use a little extra time to check the helptext, changes to EDSSharp and a better class name than Filetypes

The unified exporter

all classes that can export something to file has a new function GetExporters from IFileExporter
That function returns a array contains all the the cli/gui program needs to describe it to user and do the actual export.

The class Filetypes (suggestion on better name are welcome 🙈) uses reflection and finds all classes that support IFileExporter and uses GetExporters to make a list of all supported export formats.

Changes to EDSSharp

It should be 100% backwards compatible as long as you followed the original documentation
There are no changes to how --infile work.

--type is now optional and is only needed if the file extension in --outfile can not be used to find one exporter, if more than one or zero is found to match --type is used.

new helptext

Usage: EDSEditor --infile file.[xdd|eds] --outfile [valid output file] [OPTIONAL] --type [valid type]
The output file format depends on --outfile extension and --type
If --outfile extension matcher one exporter then --type IS NOT needed
If --outfile extension matcher multiple exporter then --type IS needed
If --outfile has no extension --type IS needed
Exporters:
  ElectronicDataSheet [.eds]
  DeviceConfigurationFile [.dcf]
  CanOpenNode [.h,.c]
  CanOpenNodeV4 [.h,.c]
  CanOpenXDDv1.0 [.xdd]
  CanOpenXPDv1.0 [.xpd]
  CanOpenXDDv1.1 [.xdd]
  CanOpenXDDv1.1stripped [.xdd]
  CanOpenXPDv1.1 [.xpd]
  CanOpenXDCv1.1 [.xdc]
  DocumentationHTML [.html]
  DocumentationMarkup [.md]
  NetworkPDOReport [.md]

@nimrof nimrof self-assigned this Aug 11, 2024
@nimrof nimrof mentioned this pull request Aug 11, 2024
@modbw
Copy link

modbw commented Aug 12, 2024

@nimrof
LGTM
Gives a lot of flexibility over #130 so I prefer this way.

@trojanobelix
Copy link
Collaborator

LGTM2
@nimrof Nice work - Thanks!

@CANopenNode
Copy link
Owner

Very nice.

Few comments:

  1. Spelling typo? ExporterDiscriptor -> ExporterDescriptor
  2. "CanOpen XPD v1.1": Standard specifies XPD as Xml Profile Definition. I think, that file is the same as XDD file, except extension.
  3. I would like new experimental protobuffer exporters to be added. If you agree.

ended up delaying changes to the GUI, it was more complicated as the canopennode version has a loot of connection to the gui,

Basically CANopenNode don't need connection to the GUI. We can keep the setting, but instead "Select exporter" dropbox, we can use checkbox "Legacy CANopen properties". I can prepare a PR for this if you like.

Please use a little extra time to check the helptext,

Maybe some extra explanation, that "type" is equal to description listed below.

The class Filetypes (suggestion on better name are welcome 🙈)

Maybe "FileExporters"? Or class could be part of IFileExporter.cs, as it is short and somehow belongs there?

@CANopenNode
Copy link
Owner

ended up delaying changes to the GUI, it was more complicated as the canopennode version has a loot of connection to the gui,

Basically CANopenNode don't need connection to the GUI. We can keep the setting, but instead "Select exporter" dropbox, we can use checkbox "Legacy CANopen properties". I can prepare a PR for this if you like.

I checked that part again. And have a suggestion.

There is a setting ExporterType and is used by:

  1. Preferences.cs: No change necessary
  2. DeviceODView.cs: no change necessary, just keep two options for the user.
  3. DevicePDOView2.cs: no change necessary (CANopenNode is strict about generated PDO objects, user should take care as now) We can also remove that dependency later.
  4. Form1.cs: just forget the ExporterType setting here and integrate the new exporter interface.

I think, this will not add any extra confusion to the user. CANopenNode V4 users won't notice the difference, except GUI in DeviceODView.cs. CANopenNodeV1.3 may have problems with wrong setting with DevicePDOView2.cs.

@nimrof
Copy link
Collaborator Author

nimrof commented Aug 31, 2024

Few comments:

1. Spelling typo? `ExporterDiscriptor` -> `ExporterDescriptor`

Thx, fixed

2. "CanOpen XPD v1.1": Standard specifies XPD as Xml Profile Definition. I think, that file is the same as XDD file, except extension.

You are correct, i misread, fixed by switching to .nxdd

3. I would like new experimental protobuffer exporters to be added. If you agree.

Yes absolutely!
I initially wanted to move it into its own class first and then forgot about it.
Fixed

ended up delaying changes to the GUI, it was more complicated as the canopennode version has a loot of connection to the gui,

Basically CANopenNode don't need connection to the GUI. We can keep the setting, but instead "Select exporter" dropbox, we can use checkbox "Legacy CANopen properties". I can prepare a PR for this if you like.

If you have the time, please do. It looks like the next few weeks will be busy for me.
Got a little overwhelmed with the GUI when i found it it affects available datatypes too

Please use a little extra time to check the helptext,

Maybe some extra explanation, that "type" is equal to description listed below.

good point, tried to fix

The class Filetypes (suggestion on better name are welcome 🙈)

Maybe "FileExporters"? Or class could be part of IFileExporter.cs, as it is short and somehow belongs there?

Totally forgot to mention it🙈, but I was hoping to get importers into the same class in the near future.

@nimrof nimrof marked this pull request as ready for review September 1, 2024 10:07
@CANopenNode
Copy link
Owner

Hi @nimrof,

I was quite busy last days, I didn't manage to check your PR. As I understand, it is finished, but not yet added to GUI menu entry. There is some complexity with "ExporterType", but I think, we should not care about it at this point.

If you will find time, you could add new exporter interface into "Form1.cs"

  1. Save, Save as.. : into xdd1.1 format
  2. Export: all others including two canopennode
  3. Optionally separate menu entry for two canopennode exporters

Just remove ExporterType dependency from that file. I will make this setting more clear and independent in separate PR.

@trojanobelix
Copy link
Collaborator

I find the generation of the CANopenNode c/h files under ‘Export’ unfortunate.

I would prefer a menu item like ‘Generate CanopeNode OD ’

justmy2ct

@nimrof
Copy link
Collaborator Author

nimrof commented Sep 25, 2024

Hi @nimrof,

I was quite busy last days, I didn't manage to check your PR. As I understand, it is finished, but not yet added to GUI menu entry. There is some complexity with "ExporterType", but I think, we should not care about it at this point.

No problem.
Okay, ignoring the rest and merging

If you will find time, you could add new exporter interface into "Form1.cs"
1. Save, Save as.. : into xdd1.1 format
2. Export: all others including two canopennode
3. Optionally separate menu entry for two canopennode exporters

Hope to get some time this weekend🤞

Just remove ExporterType dependency from that file. I will make this setting more clear and independent in separate PR.

👍

I find the generation of the CANopenNode c/h files under ‘Export’ unfortunate.
I would prefer a menu item like ‘Generate CanopeNode OD ’

Agree, we can always reconsider changing it the new gui

@nimrof nimrof merged commit 86294fb into main Sep 25, 2024
4 checks passed
@nimrof nimrof deleted the unified-fileexporter-interface branch September 25, 2024 16:38
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