Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement the remaining canopen datatypes #440
Implement the remaining canopen datatypes #440
Changes from 4 commits
1205906
edfdf69
0bc5d1c
b4fdef6
a6e7ca3
e2e53e0
1a5bd8e
0164e28
9c2ec3d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 just noticed that using these classes (just as the previous 24-bit only variants) would break with PDO that are not aligned to full bytes. That's because the code there actually examines the
.format
attribute, which is only available on the realStruct
class:canopen/canopen/pdo/base.py
Line 567 in 94f337d
Let's add a read-only
str
property to forward.format
to theself.struct.format
instance variable. Or should we instead just derive these classes fromstruct.Struct
?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.
What exactly is this test doing? It tests that the lower-case of the struct format string is not empty? So is it then a method to determine all types that are struct native, i.e. not 24, 48, 56 and so on? This code is very strange to me. The comment sais "Check if the variable is signed and if the data is negative prepend signedness". I think I need some enlightenment here.
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.
The test hinges on the implementation detail that the struct mini-language uses uppercase codes (B, H, L, Q) for unsigned and lowercase (b, h, l, q) for signed types. It then goes on to check whether the value is in the upper half of the representable number space (with the specified number of bits), which indicates that it was a negative number being truncated. In that case, padding with 1-bits is added to the left to make it a truly negative integer (in two's complement) after converting back to the full-width integer type.
This is needed for partially mapped signed integer variables. Judging from the name, the
STRUCT_TYPES
listing was meant to indicate we do have astruct
-based en- / decoder available, thus it can be assumed that the uppercase / lowercase distinction works.I think the easiest way to fix this is to actually inherit from
struct.Struct
in your generic integer classes, so that we do have access to a real.format
attribute again. Inheritance instead of composition, as an OOP specialist would put it.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.
Ahhh. It sais
islower()
notlower()
... Sorry, I misread it.I agree, let's implement some upper case letter for unsigned and lower case for signed. I think simply proxying
format
toself.struct.format
in theUnsignedN
andSignedN
object will suffice.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 really prefer the inheritance approach, as it allows us to also type-annotate the STRUCT_TYPES mapping as
dict[int, struct.Struct]
and we don't need to list theIntegerN
andUnsignedN
classes anywhere. Should also simplify these classes a bit.Should I take a stab at converting them? For example:
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.
struct.Struct
is a base type; you can safely inherit from it. Builtin types and stdlib types that are not safe to inherit from are not base types (or should not be base types). See also:I see now that that page needs some care (yes, the triple question marks...)
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.
FTR, the
struct.Struct
type spec can be found here:https://github.com/python/cpython/blob/4055577221f5f52af329e87f31d81bb8fb02c504/Modules/_struct.c#L2239-L2246
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.
Wow, it never crossed my mind that a Python class in the stdlib could NOT be subclassed. If such a thing exists, I hope there is a big fat runtime or compiler warning, besides being clearly stated in the documentation.
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.
You'll get a
TypeError
:For most user-visible types, there is a notice of some kind in the docs:Wrong link, and more importantly, the docs do not mention it in this case.
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.
Anyway (continuing the digression): most non-base types are not user visible. For example, the
_struct.unpack_iter
type:This file was deleted.