canmatrix icon indicating copy to clipboard operation
canmatrix copied to clipboard

merge features and refactor structure

Open ebroecker opened this issue 3 months ago • 5 comments

@daemonPainter @tainnok

I'll need some time for merging all your prs. And YES maybe a Version2-branch could be a way to go on with @daemonPainter PRs...

I am free and open for any option to get all of your work in this project with least work for all of you - so any comments how to do things right are wellcome!

ebroecker avatar Sep 05 '25 21:09 ebroecker

Hi @daemonPainter @tainnok

I just hat a quick look over all your prs wondering how I can merge all of them. If I do the refactoring from @daemonPainter first, the prs from @tainnok will probably not mergable and vice versa.

After my quick view I think I only can do the wrong direction ... What do you think, how to get everything in with least efford?

ebroecker avatar Sep 06 '25 11:09 ebroecker

i am not sure if i get @daemonPainter correct in #863 . i don't get the point if his refactoring is finished or a WIP.
e.g. "This is far from being completed"? Does this mean #863 is not finished? Or does this mean that the content of #863 is finished for production-usage, but the planned follow-up refactoring "is far from being completed"?

nevertheless, as my changes do not change anything the architecture of canmatrix and vice versa @daemonPainter changes in #863 does not change anything in the architecture nor the functionality itself (if i got it correct, he changed the file-structure) i think the least efford would be to merge #873 into development, then copy over all changed parts (should be fine to just copy over every method as a whole which has changed) into the changed file-structure (plus new classes and methods also, there are just two if i remember correct).

as long as @daemonPainter has not changed any functionality in #863 imho that would be the easiest way. maybe @daemonPainter can give a hint if i got it correct that no functionality was changed in #863 ???

tainnok avatar Sep 09 '25 13:09 tainnok

@tainnok

I tried to merge you large PR, hopefully did't blast everything

ebroecker avatar Sep 16 '25 12:09 ebroecker

First of all, I am sorry but I was tasked with something else for a while.

@tainnok the refactor is complete. No WIP. I have detected many gaps in the testing coverage, there is where I want to keep hammering. "Far from being completed" was a reference to my future plans.

I have not changed the functionality, however the package and module structure changed. Where you previously would have imported canmatrix (package), you now need to import CanMatrix (class). Same goes with all of the stuff inside: Pdu is now a class, not part of CanMatrix (class), thus you may import directly from canmatrix.Pdu import Pdu. Same goes for all classes.

I have done my best to avoid interfering with the functionality, and kept the original test cases as proof of that.

Known Issues

The J1939 handling. It is not working as expected, in my opinion, and its tests fail. In my PR I set them to skip.

Next steps

J1939 and CyberSecurity should be and add on, in my mind. Expanding on base classes, which they inherit, and made to do something more. I am still working out a plan. DBC handling is also something to address: add some real cases DBCs for testing. Test coverage is the next big thing. I would avoid touching the command line portion, but I'd like to address the backlog of issues and improve on the package, so that it can be used as a bootstrap for further development.

  • [ ] Continue development through test-driven design
  • [ ] Appoint @ebroecker as owner of the functionality and start pushing test cases (which will fail) as design requirements
  • [ ] Define expected J1939 behavior
  • [ ] Define expected ARXML + CyberSecurity behavior
  • [ ] Improve test coverage to 95%
  • [ ] Prioritize backlog and start fixing those issues as well

daemonPainter avatar Sep 29 '25 08:09 daemonPainter

should be merged with #879

ebroecker avatar Nov 16 '25 21:11 ebroecker