CANopenNode / CANopenEditor

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

Unified file exporter interface #131

Closed nimrof closed 1 month ago

nimrof commented 2 months ago

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 https://github.com/CANopenNode/CANopenEditor/pull/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]
modbw commented 2 months ago

@nimrof LGTM Gives a lot of flexibility over https://github.com/CANopenNode/CANopenEditor/pull/130 so I prefer this way.

trojanobelix commented 2 months ago

LGTM2 @nimrof Nice work - Thanks!

CANopenNode commented 2 months ago

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

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 commented 1 month ago

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.

CANopenNode commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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