-
Notifications
You must be signed in to change notification settings - Fork 66
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
protobuffer poc #115
protobuffer poc #115
Conversation
Protobuffer seems OK to me. proto files are very clear and cs files are generated automatically for them. I think, we should continue with it. I'm very inexperienced with configuring Visual studio, but once project is set-up correctly I can do the job with adjustment of importers, exporters, etc. Maybe you can do some more integration, It took me quite a lot of time for simple testing. Maybe we could avoid json format and use binary wireformat directly, if it is more reliable. |
ok 👍
Sure, so i should make this?
Its not more reliable, but it uses way less space and way faster to read/write. |
Maybe we can continue with this branch an make a complete transition to protobuffer. For the start, you may do as you suggest, we also have a TODO from #102: My suggestion for open/save, import/export:
Next is precisely specifying protobuffer properties and integrate it into existing importers/exporters and GUI. I don't expect, anything will be broken in CANopenEditor. I can do that part of the job, but of course, cooperation is welcome. EDS import/export will have to be rewritten. My approach for exporter would be to simply generate it as a text file, just write each property separately. If there is better approach in c#, please let me know. Similar for importer: read tokens and sort them into properties one by one. Our protobuffer should have some extra validation control. For example, OD object of type VAR should have only one subindex, etc. Maybe it is good enough, if validator just writes warnings somewhere. How could we add our validators to protobuffer, can we extend classes with our methods? |
Yes, in my head that was limited to some way to make information about filetypes and parameter passed to it
Sounds good as long as we can import/export other format via import/export
Suggestion, can we only support multiple and if there is only one then its only one
That sound good, i am still on 1.3 and very much agree moving it away from settings.
I do think the EDS import/exporter is very good use of c# tbh
To to that we must make individual messages for var, array and then put the limitation there, but there is no way to limit the size of the array to a value, if it is repeated it can be way longer than 255 so we do need extra code anyway |
Checking the validity of CANopen objects is very complex. I have done this once with the CiA conformance testing tool and found it horrible. The CANopen CC electronic data-sheet (EDS) checker developed and supported by Vector has been updated and revised and is now called CANeds. Unfortunately, it is no longer available as a stand-alone tool. The EDS Checker tool is embedded in the Vector tool chain (e.g. CANalyzer.CANopen). Unfortunately, you now have to download and install a multi-gigabyte package. But the tool is very helpful and you are impressed by all the things that actually need to be checked. You don't always want to do this yourself in your own importer. If I have problems, I always ask for the EDS or XDD and see what the CANeds CiA 306: CANopenEditor: |
Maybe yes. We always open/save a project (network) file, which contains an array of zero or more devices and some own properties (baudrate for example). But importer/exporter should include an option for single device in protobuf format. |
I don't like attributes in current EDS importer/exporter very much. But each programmer has his own style, so this doesn't matter much. For me it is the most important, that we have our protobuffer as simple as possible, should include all that is needed, well specified and also flexible. Standards have differences in details. I think, our protobuf must contain best from the standards and we should follow them as much as reasonable. But we don't have to stick with all their rules. Of course, exporters must comply them fully, but our protobuf is under our control. I wish it to be clear and logical and don't care much about differences in the standards. For example, I would like to think about EDS exporter only in EDS exporter code. I don't like to add additional "eds format" specific rules into protobuf, just to make code look more like c#. Some properties will be transferred to eds file just as they are, like "name=value", other properties (accessType) will need some extra code. Recently we had a discussion about a "revisionNumber" and there was just no simple solution for that minor issue. Besides that, eds is not the only exporter, there is also xdd. And new CANopenFD standard does not even have eds specification, only renewed xdd... |
I think, we will need some extra code around auto-generated protobuf files. This would be some helper functions, generic validators, etc. I think, also importers or exporters may have their own validators, for example CANopenNode_V4 exporter does much checking. Validator may add a warning into the "Warnings" class. We should have well defined approach for validators. But we can make more or less complex validators for separate exporters any time. |
More confusing is the Access Type = ro with a TRPDO. Would expect a TPDO. |
I pushed protobuffer importer/exporter. Both are accessible from GUI. It seems they works correctly. Tested: Load xdd file -> export to protobuffer file -> close file -> load protobuffer file -> save as xdd -> both xdds are the same, except metadata. Exporter and importer were actually added to CanOpenXDD_1_1.cs file, so translation from EDSsharp works indirectly. Nothing else was changed. Program builds normally from visual studio, runs normally in windows. I made proto definition file very slim for the start. If we will continue with it, we may add additional properties. Protobuffer file (as CANopen device description) may be very easy to use with many other languages, including python. With few lines there is a complete database with object dictionary, etc. Thank you @nimrof for the idea and the initial example. CANopenEditor is now little wider. I would like to make it much more slim (by using protobuffer as a core), but there must be a wider interest for this. Also new GUI would be necessary. We should somehow agree for the next steps. But for now protobuffer im/exporter is just enough for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for afk a while.
Only thing i really dislike is that it is added to the xdd source file and not its own file :)
One think i dislike is some of the changes in csproj (see own comments)
I understand its experimental so feel free to ignore the rest of the comments.
- Missing cli export/import
- Missing unit tests
- Could it be a good idea to license the .proto file under a permissive license like MIT? (i may not be neutral with this suggestion😇)
Rest is mostly nitpicking
EDSEditorGUI/Form1.cs
Outdated
@@ -304,6 +305,14 @@ private void openToolStripMenuItem_Click(object sender, EventArgs e) | |||
openEDSfile(odf.FileName, InfoSection.Filetype.File_DCF); | |||
break; | |||
|
|||
case ".binpb": | |||
openProtobufferfile(odf.FileName, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use odf.FileName inside openProtobufferfile to figure the type or?
EDSEditorGUI/Form1.cs
Outdated
@@ -369,6 +378,49 @@ private void openXDDfile(string path) | |||
|
|||
} | |||
|
|||
private void openProtobufferfile(string path, bool json) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void openProtobufferfile(string path, bool json) | |
private void OpenProtobufferFile(string path, bool json) |
just c# coding standard
libEDSsharp/libEDSsharp.csproj
Outdated
@@ -23,6 +23,22 @@ | |||
</EmbeddedResource> | |||
</ItemGroup> | |||
<ItemGroup> | |||
<PackageReference Include="System.CodeDom" Version="6.0.0" Condition="'$(TargetFramework)' == 'net6.0'"/> | |||
<PackageReference Include="System.CodeDom" Version="8.0.0" Condition="'$(TargetFramework)' == 'net6.0'" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't see the need to change this to v8, its sort of made for the .net version (6)
libEDSsharp/libEDSsharp.csproj
Outdated
<PackageReference Include="System.CodeDom" Version="8.0.0" Condition="'$(TargetFramework)' == 'net6.0'" /> | ||
<PackageReference Include="Google.Protobuf" Version="3.27.2" /> | ||
<PackageReference Include="Google.Protobuf.Tools" Version="3.27.2" /> | ||
<PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="6.0.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this is here?
compile just fine without it
{ | ||
switch (subEntry.Sdo) | ||
{ | ||
case LibCanOpen.OdSubObject.Types.AccessSDO.Ro: return parameterTemplateAccess.read; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case LibCanOpen.OdSubObject.Types.AccessSDO.Ro: return parameterTemplateAccess.read; | |
case OdSubObject.Types.AccessSDO.Ro: return parameterTemplateAccess.read; |
I think you can remove all/most LibCanOpen.
in this file
My point of view: protobuf is (will be) our core and my new code inside xdd is importer/exporter for xdd. Actually I wrote exporter for xdd and temporary use it as exporter from eds (to xdd) to protobuf.
It didn't work to me until I add "Unsafe" module. I did some automatic upgrade and it did what id did. Please fix it, I'm a little short here. Yes missing a lot and I agree with MIT for protobuffer. Most important question for me: Should we make a protobuf as a core database and make totally new eds importer/exporter and keep all eds related code away from protobuf. That will require update of the most of the code, but I think, it will be worth. |
Okay that makes more sense, just got hung up in ReadProtobuf & WriteProtobuf in xdd, but its a quick way to get something experimental working.
Oh, i need to do more runtime testing then
👍
agree, but slowly pls :) |
I made xdd exporter/importer first, because this file is most familiar to me as an author.
Yes, I agree. I only ask you to fix this PR and then we merge it into main. So we have experimental protobuffer exporter/importer. For me this is quite a lot, because there are many possibilities to play with it. If we once decide to put CANopenEditor on the next level, my proposal is the following:
If we decide. However, current version is good enough, bud I don't feel adding more functionality in it. Anyway, I wish we have a modern and rich CANopen GUI tool once in the future. Or maybe set of simpler tools? Slowly. |
okay np👍(once i understood why)
Since its experimental i think we can merge now,
Sounds good
|
I tried again and deleted |
Sounds good |
protobuffer test code for #112
I have translate it mostly 1:1 but divided it a little. Feel free to ignore that.
Testcode
result