christiansandberg / canopen

CANopen for Python
http://canopen.readthedocs.io/
MIT License
424 stars 191 forks source link

Support canfestival OD-files? #481

Closed sveinse closed 6 hours ago

sveinse commented 4 days ago

This is a wild shot: Canfestival is a open-source implementation of CANopen and it has its own file format and editor for OD files, https://github.com/Laerdal/python-objdictgen and on PyPi https://pypi.org/project/objdictgen/ (disclaimer: I am maintainer for it). To be able to use the canfestival OD-files with canopen, a file reader and writer for loading the OD must be implemented. The dilemma is that this importer is not really on-topic for canopen since canfestival is a 3rd party library for canopen. However, the opposite is also true: It doesn't really belong in the python-objdictgen package, as canopen is 3rd party for it.

Any suggestions from this camp what is best approach for this?

acolomb commented 4 days ago

I see no reason why we should not add support for another format. It's open source and therefore an open specification related to CANopen. We're not restricted to implement only CiA standards in this library, IMO.

And we actually do already support some "extensions" to the OD records, namely the factor and physical unit conversions.

Does direct support for the format bring any advantages over passing through the generic standard EDS code-path? I mean exporting EDS from objdictgen (assuming it is supported) and importing that in canopen.

Is this maybe a case for the canmatrix package, where different proprietary CAN-related data formats can be converted?

erlend-aasland commented 4 days ago

How about introducing a OD converter API instead?

sveinse commented 4 days ago

Does direct support for the format bring any advantages over passing through the generic standard EDS code-path? I mean exporting EDS from objdictgen (assuming it is supported) and importing that in canopen.

That's a good point. For the use in canopen, the format doesn't add anything new that what EDS does. Since import_od() supports passing an fp-like object, so if objdictgen is able to export EDS into a StringIO object then a conversion can be done with one operation in-memory without the need for a special converter. 🤔

The reason canfestival use its own format because it contains additional OD metadata which the EDS format doesn't have that the code generator utilize. Such as user provided comments.

acolomb commented 4 days ago

The reason canfestival use its own format because it contains additional OD metadata which the EDS format doesn't have that the code generator utilize. Such as user provided comments.

Since we already have custom EDS extensions here as well, that could also be a possible way to interoperate, without a full-blown new format parser. You're in the right position to implement such extensions on both sides, and make it a de-facto standard. Might even consider proposing it to CiA for a future version of their standard 306.

acolomb commented 4 days ago

Another side-note: There is also https://github.com/CANopenNode which uses a C-based format to define object dictionaries with extended information. Not sure there is any value in supporting that instead of a common EDS path though.

sveinse commented 6 hours ago

I'm closing this for now. Thank you for the discussion.