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

Upgrade to net6 #77

Closed
wants to merge 12 commits into from
Closed

Upgrade to net6 #77

wants to merge 12 commits into from

Conversation

nimrof
Copy link
Collaborator

@nimrof nimrof commented Dec 22, 2023

Upgraded to .net 6 and its a big pr. :/

did some testing and afik there are some is the only problems

  • The pdo mapping combobox is not wider than the column, this is no longer supported 38ddd38
  • i may have bugged something when i fixed problem with report on .net 6 af6effd
  • i may done something very wrong while fixing css injection the web report viewer ce955bc
    no export in com, there is a loot of warnings regarding com, but it looks like it works

There is loot of warnings and messages while compiling. Have not fixed any of them as that would make this pr. bigger, but can do it now
There are also some parts of the project files that can be cleaned up, but that would also make the pr. bigger and harder to follow, but can do it now

Comments?

@trojanobelix
Copy link
Collaborator

Thanks for this contribution. I started to upgrade to VB6 but never found the time to finish it. I will try to incorporate it into the VB6 branch in the next few days.

@nimrof
Copy link
Collaborator Author

nimrof commented Dec 28, 2023

Thanks for this contribution. I started to upgrade to VB6 but never found the time to finish it. I will try to incorporate it into the VB6 branch in the next few days.

Nice, but no hurry on my part so enjoy spare time/holidays the way you want

@trojanobelix
Copy link
Collaborator

created a new branch (github-desktop-nimrof/upgrade-to-net6) with this PR. Please test.

@trojanobelix trojanobelix marked this pull request as ready for review February 20, 2024 07:43
@nimrof
Copy link
Collaborator Author

nimrof commented Feb 20, 2024

created a new branch (github-desktop-nimrof/upgrade-to-net6) with this PR. Please test.

Do you have a list of what to test?
Its a little hard to follow why some of the changes is done :)
7392e90 bugfix something?
eaa8513 bugfix something else?
e913d10 nodeguarding something?

@trojanobelix
Copy link
Collaborator

I have merged the net6 support into the bugfix branch. This contains some fixes (which don't clash with your changes as far as I could see). Also support for nodeguarding. Nodeguarding was recently included in V1.3 and in the latest main branch of CANopenNode.

The latest commits were for exceptions I noticed during the Net6 tests ("extensions" is a typo, sorry).

If you would like to support the CANopenEditor and maybe more often, I can ask Janez to make you a contributor. Help is very welcome.

@nimrof
Copy link
Collaborator Author

nimrof commented Feb 21, 2024

I have merged the net6 support into the bugfix branch. This contains some fixes (which don't clash with your changes as far as I could see). Also support for nodeguarding. Nodeguarding was recently included in V1.3 and in the latest main branch of CANopenNode.

Code looks good to me, will test a little bit.

The latest commits were for exceptions I noticed during the Net6 tests ("extensions" is a typo, sorry).

np, it makes sense now. good catch

If you would like to support the CANopenEditor and maybe more often, I can ask Janez to make you a contributor. Help is very welcome.

That sounds tempting, but just a few things i want to be open about first.

  • Currently working on a c# opensource canopen stack, if you or anyone else sees this as a conflict-of-interest then i am not interested.
  • I really want to focus on improve the project by introducing automated build & testing on github and changing issue templates.
    If that is something that is okay then good.
  • Don't want to "reinvent the wheel" so i want to use many test cases from my c# project code in c,
    No problem signing over copyright to the c test code or anything like that, just want too make it clear where the test cases will comes from.
    As long as everyone is okay with this then good.
  • I might not be more active :(

@trojanobelix
Copy link
Collaborator

@CANopenNode Not a problem for me, what do you think?

@nimrof The clean-up is certainly a good idea. Structurally, there is certainly room for improvement.
Janez also has some ideas.
In my view, we should first agree on the goal of further development and then see whether it goes in the same direction.

@nimrof
Copy link
Collaborator Author

nimrof commented Feb 21, 2024

we should first agree on the goal of further development and then see whether it goes in the same direction.

Sound good too me

@nimrof
Copy link
Collaborator Author

nimrof commented Feb 26, 2024

Looks good to me, just a few things:

  1. Looks like the line got splitt

    The life time factor multiplied with the guard time gives the life time for the Life Guarding Protocol. It is
    0 if not used.</description>

    <description lang="en">The life time factor multiplied with the guard time gives the life time for the node guarding protocol. It is
    0 if not used.</description>

  2. The 'object viewer' does not resize correctly, does it look good in your setup @trojanobelix
    If i just open it in visual studio and save it again if looks good, but that changes a loot of lines.
    image

  3. The number of monitored node might look better if it is a little smaler or text over it?
    image

edit: clarification

@CANopenNode
Copy link
Owner

@CANopenNode Not a problem for me, what do you think?

@nimrof The clean-up is certainly a good idea. Structurally, there is certainly room for improvement. Janez also has some ideas. In my view, we should first agree on the goal of further development and then see whether it goes in the same direction.

A have no problems, there is no conflict-of-interest, more CANopen is better for me. Also I would like automated build & testing, clean-up, etc.

My only demand is, to have a reliable CANopenNode exporter. I'm open for other improvements.

It was not my project from the beginning, I'm not very skilled with c#. Mine is importer/exporter CanOpenXDD_1_1, CanOpenNodeExporter_V4 and some improvements in other parts. That was quite robust for me. I didn't want to change other things much (for backward compatibility?).

I don't like the central part, which is now eds.cs file. I would prefer to turn eds.cs into an importer/exporter and make new central part with more clear database definition, especially for object dictionary. Unfortunately, standards are not very simple, there are electronic data sheets in eds files (ini file type) or in xdd files (xml file type), different versions. But actually object dictionary database is not that complex.

Currently only reliable importer/exporter is CanOpenXDD_1_1, others do not save CANopenNode specific data correctly. But this is complex file. See here for some description: https://github.com/CANopenNode/CANopenNode/blob/master/doc/objectDictionary.md#xml-device-description-xml-device-description


@trojanobelix, I merged bugfix branch into main and made new branch "upgrade-to-net6" with this PR. Maybe other branches could be removed?
I'm adding @nimrof as a developer, I think, it is necessary to apply some build and testing automation. 'main' branch should stay on the old version for a while and feel free to add improvements on the other branch. And if the goal of the further development will change, we should agree before.

@trojanobelix
Copy link
Collaborator

Thanks for the merge. I'll tidy up the branches when I get a chance. It was on my list anyway.
Main stays as it is for now. The current version has a few rough edges, but I use it productively and get on well with it.

I'm not a C# hero either and my aim was to make the TX RX mapping tab usable. It now works satisfactorily and makes my work easier. No more and no less.

I would be happy if we could get support from @nimrof.

@nimrof
Copy link
Collaborator Author

nimrof commented Mar 11, 2024

Hi,
First off i am no c# hero, but might be enough and happy to help :)

And if the goal of the further development will change, we should agree before.

Sounds good.

So something like this:

  • make epic/issue
  • we agree it sounds good
  • make pr(s).
  • get one/or two? approval
  • merge with main?

'main' branch should stay on the old version for a while and feel free to add improvements on the other branch.

I would not work directly on main branch, but i think it would be beneficial to get the test and automation working on main branch, that way we can hopefully catch bugs when they are still pr.
I can limit myself to only touching test & github code.

Is that something that could be merged into main via pr. in the not so distant future ?

If not then that is okay too, i am just trying to find a workflow :)

@CANopenNode
Copy link
Owner

I made some git related job:

  • new branch "Net4.8", currently the same as "main". Also added tag "v4.2" there.
  • generate binaries for the latest Net4.8 branch and put it into "build" branch. (This may change in the future.)

One advantage of Net4.8 is, that binaries can be simply run in Linux in Mono. Net6 does not work there directly. But this does not matter much.

Branch "Net4.8" could stay in the old version, there are possible some fixes, if necessary.

So maybe it is not necessary for "main" to stick on the old version. If current "upgrade-to-net6" branch is stable enough, "main" could be set to there. However, we should take care, that "main" won't stuck on some partly finished code. Otherwise, please don't limit yourself.

@trojanobelix
Copy link
Collaborator

@CANopenNode Totally agree

@nimrof
Automated tests are certainly a good thing.
I don't have the experience with dd, but integrating it into the main branch certainly makes sense.

Perhaps we should also publish binaries as stable releases and the latest version as a test release (or whatever it's called).
I hope this will lead to more feedback on possible bugs. Not everyone can or wants to compile the code themselves.
That would be in what you calls "github code", wouldn't it?

@trojanobelix
Copy link
Collaborator

Looks good to me, just a few things:

  1. Looks like the line got splitt
  1. The 'object viewer' does not resize correctly, does it look good in your setup @trojanobelix
    If i just open it in visual studio and save it again if looks good, but that changes a loot of lines.
    image
  2. The number of monitored node might look better if it is a little smaler or text over it?
    image

edit: clarification

  1. Agree, I will change it
  2. What resolution do you use? It is okay on my side.
    image

@nimrof
Copy link
Collaborator Author

nimrof commented Mar 12, 2024

One advantage of Net4.8 is, that binaries can be simply run in Linux in Mono. Net6 does not work there directly. But this does not matter much.

Good point. afik you can run .net6 code that uses windows forms with wine.
command line util should without any problems with .net 6 installed

So maybe it is not necessary for "main" to stick on the old version. If current "upgrade-to-net6" branch is stable enough, "main" could be set to there.

maybe, i will let you and trojanobelix decide.

However, we should take care, that "main" won't stuck on some partly finished code. Otherwise, please don't limit yourself.

Agree

Perhaps we should also publish binaries as stable releases and the latest version as a test release (or whatever it's called). I hope this will lead to more feedback on possible bugs. Not everyone can or wants to compile the code themselves. That would be in what you calls "github code", wouldn't it?

That make sense to me, the users are probably c/c++ developer so if we can save them from learning how to compile c# that would be a good thing

  1. What resolution do you use? It is okay on my side.
    image

Strange.
I use 1920x1080 at the moment

@trojanobelix
Copy link
Collaborator

@nimrof. Regarding your last commits: I am not an expert in the .Net environment. I am at home in the embedded world. So I am very grateful to you for supporting the project.

I can therefore not contribute much to the review in this area. So don't be surprised about the lack of comments.

I am very happy that things are continuing at this level and hope to see more exciting new ideas from you.

@trojanobelix
Copy link
Collaborator

Unfortunately, I'm not a Github guru either. I would find it very helpful to be able to provide bugfix and production releases as a binary. Shouldn't be a problem, this is Github RNS, but unfortunately I don't know the normal workflow that well. So if you already have experience with it, that would be a good thing....

If not, that's not a problem either. The project is running, we have other hobbies too ;-)

@nimrof
Copy link
Collaborator Author

nimrof commented Mar 28, 2024

@nimrof. Regarding your last commits: I am not an expert in the .Net environment. I am at home in the embedded world. So I am very grateful to you for supporting the project.

Thanks for the kind words.

I can therefore not contribute much to the review in this area. So don't be surprised about the lack of comments.

No problem :)

I am very happy that things are continuing at this level and hope to see more exciting new ideas from you.

Thanks, but do not expect this level of commits to continue :)

Unfortunately, I'm not a Github guru either. I would find it very helpful to be able to provide bugfix and production releases as a binary. Shouldn't be a problem, this is Github RNS, but unfortunately I don't know the normal workflow that well. So if you already have experience with it, that would be a good thing....

I have not done it automatically before, but i need to do it on my other c# canopen project so it might be good time to take a look, just want to get the dualbuild into main first. (1-3 pr. away)

If not, that's not a problem either. The project is running, we have other hobbies too ;-)

👍

@CANopenNode
Copy link
Owner

I think, @nimrof is doing a great job. I actually never had a full track on CANopenEditor internals and so I'm very grateful for those updates, even if I can't verify everything. (With CANopenNode repository I'm much more nagging, because I don't like unnecessary changes there).

My philosophy for myself and others is something like, use as low rules as possible. And to many (unnecessary) discussion just takes time and energy. So I see no problem, even if @nimrof makes some cuts if he thinks, it is useful. Anyway, in the worst scenario, it is still possible to split git repo at some point.

I make some tests of binaries occasionally. As long as I get the same behavior and same outputs, nothing changes for me.
@trojanobelix, If you do the same and updates don't break your workflow, I think, we are safe and it is enough to approve the PR.


About dualbuild and v4.8 support, I think, it would be nice to have it, but only, if implementation is simple. If there are complications, I suggest just to forget v4.8.


I suggest one other thing for this point. There are two formats, which are obsolete:

  1. CanOpenXML.cs and Bridge.cs - this is importer/exporter for very old xml project file, specific for very old CANopenNode editor, not available very long time.
  2. CanOpenXDD.cs - this is importer/exporter for the old version of CANopen xdd standard. It is not maintained, not updated and not usable. 1_1 is used instead.
    I suggest to remove those files and to purge everything related to them and to forget about them. If anyone will need conversion from those old formats, he could use the old version of CANopenEditor. I didn't do that, because I'm not so skilled with Visual studio and it would be a hard job for me.

Maybe there are some other unused parts of the code. I just prefer cleaner code, so it is easier to keep track and to maintain.


And another wish, to make life easier for all of us:
eds.cs, another large file. I would like (someone) to separate importer/exporter to CANopen eds file from it (ini style). So eds.cs (or some other name), will contain only CANopen device database with all necessary methods.
After that I would like to cooperate, so we make eds.cs more clear, remove duplicate methods all-around, etc.

@nimrof
Copy link
Collaborator Author

nimrof commented Mar 28, 2024

About dualbuild and v4.8 support, I think, it would be nice to have it, but only, if implementation is simple. If there are complications, I suggest just to forget v4.8.

btw #94
Feel free to disagree, but i think it would be best to keep 4.8 until we have c# native multiplatform gui (built on avalonia or something else) up and running.
That way we can keep adding new stuff and non-windows user can keep using the newest features.
As long as we have .net6 support i can use my modern tools and i am happy.

I suggest one other thing for this point. There are two formats, which are obsolete:

  1. CanOpenXML.cs and Bridge.cs - this is importer/exporter for very old xml project file, specific for very old CANopenNode editor, not available very long time.
  2. CanOpenXDD.cs - this is importer/exporter for the old version of CANopen xdd standard. It is not maintained, not updated and not usable. 1_1 is used instead.
    I suggest to remove those files and to purge everything related to them and to forget about them. If anyone will need conversion from those old formats, he could use the old version of CANopenEditor. I didn't do that, because I'm not so skilled with Visual studio and it would be a hard job for me.

I can take a look.
Now that i got support for my modern tools i want to focus on fixing warning&complains from the build system and general cleanup.

Maybe there are some other unused parts of the code. I just prefer cleaner code, so it is easier to keep track and to maintain.

There are at least some unused reference, but i am still a little nervous to change too much before more test are in place.

And another wish, to make life easier for all of us: eds.cs, another large file. I would like (someone) to separate importer/exporter to CANopen eds file from it (ini style). So eds.cs (or some other name), will contain only CANopen device database with all necessary methods. After that I would like to cooperate, so we make eds.cs more clear, remove duplicate methods all-around, etc.

That is a big job, it looks like the import/export is not that easy to move away, but agree that it is needed.

@CANopenNode
Copy link
Owner

That is a big job, it looks like the import/export is not that easy to move away, but agree that it is needed.

Yes it is a big job. But I can take major part of the internals. I have some experience with the standards, usability of the CANopen elements, etc. I can do this with some cooperation/discussion, as I'm not very familiar with the (modern) approaches in dotnet.

Modernization of CANopenEditor would have a great usability also for my work. With current state of CANopenEditor I was never really satisfied - it is a useful and rich tool, but it was too messy to add functionalities. For example, when I was adding V4 exporter, I had to add/duplicate some methods(add mess) in eds.cs, but never really dive into the internals. And now, before we add new functionalities, it is time to make a clear and reliable foundation. Eds file reader and generator needs a review anyway.

@trojanobelix
Copy link
Collaborator

trojanobelix commented Mar 30, 2024

I fully agree with that. At the moment, we have a usable and functioning state. But it is too opaque and complex for further developments.
A cleanup would be good. And I think, we are on a good way.

@trojanobelix
Copy link
Collaborator

Maybe the resharper extension can take care of some of the boring stuff. Does anyone have experience with this?

@nimrof
Copy link
Collaborator Author

nimrof commented Mar 31, 2024

Maybe the resharper extension can take care of some of the boring stuff. Does anyone have experience with this?

I got no experience with it.
Any specific features you like?

@nimrof nimrof mentioned this pull request Apr 3, 2024
@nimrof
Copy link
Collaborator Author

nimrof commented Apr 3, 2024

Closing as we now have .net 4.8.1 and 6 side by side

@nimrof nimrof closed this Apr 3, 2024
@nimrof nimrof deleted the upgrade-to-net6 branch April 3, 2024 20:08
@trojanobelix
Copy link
Collaborator

Maybe the resharper extension can take care of some of the boring stuff. Does anyone have experience with this?

I got no experience with it. Any specific features you like?

No. I will give it a try.

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.

3 participants