Server sdo block upload (based on v2.3.0)#559
Conversation
sveinse
left a comment
sveinse
left a comment
There was a problem hiding this comment.
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.
| # to switch to regular upload | ||
| logger.info("Received block upload, switch to regular SDO upload") | ||
| self.init_upload(data) | ||
| def block_upload(self, request): |
There was a problem hiding this comment.
Function docstring and type hints.
|
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. |
|
@H-Grobben you have an indentation error in 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
left a comment
acolomb
left a comment
There was a problem hiding this comment.
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 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? |
|
@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. |
|
@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. |
|
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. |
|
I would like to help to make this PR merge ready. (Btw. I am @friederschueler, I am just forced to use my corporate account from now on) |
|
Hi @bizfsc, I figured that was probably the background :-) Happy if you can pick this up. Real-world testing was one of the important points above IIRC, so are you up to any of that currently? |
|
Yes, I have some real world devices and software stacks I can use to test this. Also I will use my end-of-month tokens to generate some tests :) |
|
We had the need more server block transfer. Busy with coding, maybe I (or my colleague) have some time to fix the tests and linters. |
H-Grobben
force-pushed
the
server_sdo_block_v2.3.0
branch
from
5ef4771 to
60255bd
Compare
|
Rebased to latest HEAD, added tests and checked coverage reports. Please check if it needs some improvements. |
test_sdo: increase coverage
|
Currently reviewing this, but a lot of changes. |
bizfsc
left a comment
bizfsc
left a comment
There was a problem hiding this comment.
Thanks for the continued work on this PR. The block transfer implementation is solid and the test coverage has improved significantly. I've identified a few issues that need to be addressed before merge.
Critical Issues
1. Bug: from_string() returns SdoAbortedError instead of int
from_string() declares -> int but returns SdoAbortedError(code). This means self.code ends up being an SdoAbortedError object when initialized from a string. mypy catches this. See inline suggestion.
2. __eq__ violates Liskov Substitution Principle
The __eq__ override should accept object as the parameter type and return NotImplemented for unsupported types (Python convention for comparison operators), rather than raising TypeError. See inline suggestion.
3. Breaking change: commenting out readinto(), readable(), tell() in ReadableStream
These methods are part of the io.RawIOBase interface. Commenting them out is a breaking change for any code that calls readable(), tell(), or readinto() on a ReadableStream. If they're not needed, remove them entirely — but consider that tell() and readable() are standard stream methods that consumers may rely on. At minimum, keep readable() and tell() since they're trivial and expected on readable streams.
Also, @acolomb's earlier review applies here too: don't leave commented-out code in the tree.
4. logging.debug() vs logger.debug() — inconsistent
block_upload() and _SdoBlock.update_state() use logging.debug(...) directly instead of the module-level logger. This bypasses the logger hierarchy and prevents per-module log configuration. See inline suggestion.
Minor Issues
databyte != Noneshould bedatabyte is not None(PEP 8, identity check)from canopen.sdo.constants import *intest/test_sdo.py— wildcard imports in test code pollute the namespace; prefer explicit importsself.crc = CRC_SUPPORTED if (docrc & _req_crc) else 0—docrcis alwaysFalse(default param), so CRC is never actually enabled. Is this intentional? If so, a TODO or comment clarifying the plan would help.SdoBlockExceptionas a bare subclass ofSdoAbortedErroradds no value if it's always caught asSdoAbortedErrorinon_request(). Consider removing it and usingSdoAbortedErrordirectly with appropriate codes.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- from_string now returns an int() - __eq__ fixed: use NotImplemented and update test - Uncommented not used code, and start using it: add test code for these uncommented lines - sorry for logging.debug:, should always have been logger.debug Minor: - None comparison fixed - import in test now specific - CRC in block is not implemented, add a TODO statement in __init__ comments of _SdoBlock - removed the bare subclass exception, not needed anymore (was usefull during debug)
acolomb
left a comment
acolomb
left a comment
There was a problem hiding this comment.
Thank you for investing more time to get this PR updated. Sorry to say this, but I'm not satisfied with the overall code structure and quality. The LocalNode stuff already feels like a tacked-on hack in some places. But this protocol implementation largely doesn't even try to blend in with the rest of the code. Instead adding helper code for doing things differently or circumventing existing solutions, which enlarges the diff significantly. It exists in its own world. Where efficient APIs for accessing larger objects are missing, it makes another copy of the data instead of trying to fix the deficiency (which is solved elegantly on the client side).
We need some substantial restructuring and architectural rethinking here, before this can be included in the library. Not to mention the current expectations regarding code quality for added features, having clear type annotations as a minimum for robust development. Coding style / formatting is not worthwhile to address now, given the bigger picture issues. Some inline comments added to highlight where I see problems, hope that helps as a bit of guidance on expected style as well.
I haven't looked at the unit tests at all. They may be valuable on their own, and could be included independently. Marked as expected failure of course, until the actual block transfer implementation is included.
| } | ||
|
|
||
| def __init__(self, code: int): | ||
| def __init__(self, code: Union[int, str]): |
There was a problem hiding this comment.
I'm not happy with this usage pattern. We should stick to one style for using the SDO abort exceptions. The established way is construction via explicit numeric IDs. We have defined constants for all of them, so please use those instead of hard-coding strings or using magic number literals. I'm aware that the constants were added after this PR was initially proposed.
This will make the PR more focused, as the changes to SdoAbortedError are not needed at all.
| self._index = None | ||
| self._subindex = None | ||
| self.last_received_error = 0x00000000 | ||
| self.sdo_block = None |
There was a problem hiding this comment.
Type hints for newly added code, please. Since there is already a BLOCK_STATE_NONE value, do we really need to add another possible state .sdo_block = None instead of resetting the existing object?
|
|
||
| def on_request(self, can_id, data, timestamp): | ||
| command, = struct.unpack_from("B", data, 0) | ||
| logger.debug("on_request") |
There was a problem hiding this comment.
This debug message is very generic and thus unlikely to help any user.
| except Exception: | ||
| self.sdo_block = None | ||
| self.abort() | ||
| raise |
There was a problem hiding this comment.
This appears to be bad API design, if the possible exception subclasses from .process_block() are not clearly documented and need such a catch-all.
The self.sdo_block = None could be moved to self.abort() to make sure we never miss resetting the state when a transfer is aborted.
Overall, this should be integrated into the ccs matching below, not open up its own SDO handling subculture. The catch-all for not crashing the receiver thread is already implemented on that level.
| if isinstance(abort_code, SdoAbortedError): | ||
| abort_code = abort_code.code |
There was a problem hiding this comment.
Although there are no type hints on this method, it's rather clear that the argument should be an abort code. We don't need more flexibility in using internal helper methods, but new code adhering to the established calling conventions. As is, this has a small runtime overhead without any added benefit in code clarity. Exceptions should be raised, not passed around, in most cases.
| self.req_blocksize = request[4] | ||
| if not 1 <= self.req_blocksize <= 127: | ||
| raise SdoAbortedError("Invalid block size") | ||
| self.data = self._node.get_data(index, subindex, check_readable=True) |
There was a problem hiding this comment.
The whole point of SDO block transfer is to support rather large objects. Buffering the complete data in memory is an avoidable waste of resources. We already fail to properly support streaming access in the existing SDO segmented transfer code / storage in LocalNode. But duplicating the data for a whole block here is going in the wrong direction.
| If False (default), initialise for block upload (server sends data). | ||
| """ | ||
| command, index, subindex = SDO_STRUCT.unpack_from(request) | ||
| # only do crc if crccheck lib is available _and_ if requested |
There was a problem hiding this comment.
Where is this "crccheck lib" actually searched for or used?
| self.req_blocksize = 127 | ||
| self._data_buffer = bytearray() | ||
| if command & BLOCK_SIZE_SPECIFIED: | ||
| (self.size,) = struct.unpack_from("<L", request, 4) |
There was a problem hiding this comment.
Should use a predefined struct, like in other places. Avoid hard-coding the offset.
| # Server defines the block size for download (client sends this many | ||
| # segments per block before waiting for an acknowledgement) | ||
| self.req_blocksize = 127 | ||
| self._data_buffer = bytearray() |
There was a problem hiding this comment.
The size extracted below (if available) should be used to pre-allocate a buffer, instead of always extending it with each new segment, possibly causing frequent reallocations. A streaming API into the local data store object could mitigate this completely, whether or not a size is given in advance.
| "Data can not be transferred or stored to the application because of the present device state" | ||
| ) | ||
|
|
||
| def get_upload_blocks(self): |
There was a problem hiding this comment.
This definitely duplicates the data for one block in memory, including the protocol overhead. A generator function or any other incremental processing would be in order.
5 participants
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.