Server sdo block upload (based on v2.3.0)

[H-Grobben]

Based of version v2.3.0, added the block transfer for SDO Server, upload.

I've worked on this for version 2.1.0, but never got around to make a PR for this. Now I've updated to 2.3.0 lately it was time to do a PR for it.

Did add a test to the SDO unit tester. Don't know if it needs to be tested more.

[sveinse]

Thank you for the PR! We really appreciate it.

I've mostly been looking at the coding aspects of the PR. As mentioned in the comments, there are many formatting changes, making it hard to evaluate the functional change (SDO block upload). I recommend doing the formatting improvement separately and first.

Some of these comments might seem like nitpicking, but we are generally working on improving the general quality of the project. It helps a lot if all contributions do the same. Just ask if there are any questions.

[H-Grobben]

I updated the PR with the review points. Thanks for the input, and sorry for the black formatting. Company has switched to it just this year, so I need some getting used to the automatic formatting.

[acolomb]

@H-Grobben you have an indentation error in exceptions.py and an unrelated breaking change in test_sdo.py. Please make sure to run the test suite locally (just call pytest from the top-level of your clone) to avoid such mistakes.

If you had given permission for the project maintainters to commit to your PR branch, I would have fixed those two already. It's just a small checkmark to set when creating the PR.

[H-Grobben]

@H-Grobben you have an indentation error in exceptions.py and an unrelated breaking change in test_sdo.py. Please make sure to run the test suite locally (just call pytest from the top-level of your clone) to avoid such mistakes.

If you had given permission for the project maintainters to commit to your PR branch, I would have fixed those two already. It's just a small checkmark to set when creating the PR.

Sorry for the indent, don't know why I missed that. Plain stupid.. But it's fixed.

For the access to the branch: external access is not allowed by the company (allowing external push/updates to the company repository) so i don't have the checkbox, and I can't find any other option to share it as editable for you, sorry.

[acolomb]

Sorry for not looking at the implementation more closely so far. I've just checked the latest changes (see inline comment), then went on looking at the feature as a whole.

I still can't say much about the overall architecture, whether the extra class SdoBlock is needed at all, or should better be integrated directly. What I did notice is that the code overall shows some sloppiness, not adhering to PEP-8 and other code quality standards for example. As previously mentioned, complete typing info would be much desired for new code. Some things are added but never actually used. The logging bypasses the module-wide logger object sometimes, and gives very terse debug message contents, which doesn't blend in with the rest of the library. The code has some more remaining whitespace issues, although not functionally relevant, but introducing needless noise.

I just want to know whether you're motivated to fix these issues and beat the code into good shape. Surely someone can help you with these code styling details. That would be easier if we had push access, but I understand why that's impossible in your case.

If you don't have the time for such details, would you still be willing to thoroughly test an implementation proposed in another PR which takes your approach and possibly does some details differently?

[acolomb]

In contrast to the others, this is not a struct.Struct object, why?

And it seems to be unused.

[H-Grobben]

To be honnest: I wrote this code some years ago, made it work for our application, and stumbled across it missing in my new PC (unittests on code not running anymore).
So I made a PR with that old code fitted into release 2.3.0. So I can only guess what was in my mind then.

The reason for putting it in a contained class is that the block transfer has some running states to keep (as the command byte does not adhere to normal SDO transfers for expedited or segmented). I did some more sniffer and test code back then and this is a result of all the combined work (it works with the CanOpenNode C firmware on several microprocessor brands).

And: yes we would test it, but I don't know in what timeframe that would be, as this piece of code does the job for us. But getting block transfers into the main branch would be beneficial if I or any of my colleagues would again forget to install our canopen lib and install the public code base.

[sveinse]

@acolomb looking at the comments from codecov in the review, there are many (new) lines which are not covered by any tests. Where are with that on new contributions like this?

[acolomb]

@sveinse frankly I haven't understood what the Codecov reports are trying to tell me. Just not that much into automatic code metrics measurement, but my focus is more on my own logical judgment of the code.

That is not to say I'm satisfied with adding code that is not covered by unit tests. But I think we have more urgent matters to tackle than reaching 100 % coverage. For new code, of course the best time to add tests is now, while we're looking at it. In that regard, I see it as a helpful indicator to judge how much effort the contributor put into (automated) testing. Real-world testing might however be even more important for this library.

[sveinse]

@acolomb on a general note (and somewhat a derailment in the PR): When migrating an existing project with no or little test coverage, its always good to state "from now on, we require 100% or >90% test coverage on commits". Now is the time that contributors have their head in this particular code, so its less effort to have it done now. Going back later to make test code for it is very hard to do.

Codecov is telling us the code coverage of the changed files in the PR, limited to the PR. That's what's great about it, because it can be an instrument to steer towards increasing the test coverage on contributions. -- But for some reason, this particular branch/PR isn't showing up right. It returns "No report found to compare against", so I don't really know what goes on here.

[acolomb]

It may be because the branch was forked before the Codecov integration went live?

Sure, aiming for nearly complete coverage at this time is the Right Thing to do. Just need to cut corners when we see the developer / reviewer capacity is not sufficient to reach it. In that case, I'd rather have new code with < 100% coverage, but clean and readable style, than no new code at all, blocked by missing tests.