Implement support for i24/u24 (#61)#62
Conversation
rohel01
force-pushed
the
u24-i24-support
branch
from
18e158a to
3cbf24b
Compare
|
On this one, I am not sure I have fully understood what needs to change to support new datatypes. |
|
One question that comes to mind is whether we store non-standard bit widths as the next largest rust type, or using e.g. arbitrary_int. I do think this could get a little involved. We have to work through what SDO transfers look like with new sizes. Also, PDOs right now are making the simplifying assumption there every mapping is a multiple of 8-bits. |
So my first implementation used the next largest rust type, but now that I have added integration tests, I see the issue: the current scalar field implementation (in object_dict/sub_objects.rs) relies on the rust type to perform size checks. I could add an extra argument to the macro to capture the "real" field size, but I believe it would be cleaner to model new field types with... types. So, I am currently exploring using the arbitrary_int crate as you suggested.
This should not be an issue right? All CANopen standard type sizes are multiple of 8-bits. |
Makes sense to me.
No...for example there is signed/unsigned24 :). Actually, I think it is possible as far as the protocol is concerned to only copy some bits to a PDO, e.g. I think you can map just the bottom 12-bits of a 16-bit object. The size of each mapping is specified in bits, not bytes. When I implemented it, I just decided to reject non-byte-aligned mappings to keep things simple, assuming I would go back to it later as needed. I guess we could continue in that spirit and implement 24-bit types but enforce the constraint that they cannot be PDO mapped. |
Sorry, I still do not understand this. signed24 and unsigned24 types use 24 bits as specified so they fit on 3 bytes (3*8=24). Why do you think they are not multiple of 8 bits? I just pushed a working implementation of SDOs with I24 and U24 data, including tests. Can you start looking at the code? I believe I might be introducing some technical debt. For example, I see the storage size of types seem to be specified in multiple places. |
No, don't be sorry. This was just me failing to engage brain. You're right, of course. I thought maybe there was a 12-bit type...but no...not that either!
Great! I will have a more detailed look soon! |
|
PDOs with u24/i24 seem to be working out of the box. Updated tests to showcase this |
| } | ||
| } | ||
|
|
||
| macro_rules! impl_arbitrary_int_field { |
There was a problem hiding this comment.
Why do we need a separate storage type here? Can't we just use the existing macro and store it as a ScalarField<u24>? The arbitrary_int::u24 already handles storage right? And... I think pushing the u24 creation up moves a panic path out of the common crate which makes it more visible and -- I'm not sure, maybe LTO handles it anyway -- but maybe more likely for the compiler to be able to prove it can't panic and remove it.
There was a problem hiding this comment.
OK...I see one snag is read_size needs a different implementation because we can't do size_of::<u24>(). Hmm...
There was a problem hiding this comment.
I don't love duplicating the same read/write methods. I pushed a commit with an approach that uses a trait to define the read size for the types, and then we can share a single impl_scalar_field macro. Let me know if you see any issue with that.
There was a problem hiding this comment.
Yes, this is the technical debt I was referring to. I have extended your approach to also prevent duplication in the SDO client. However, this is still not optimal, see get_rust_type_and_size in codegen.rs where type sizes are duplicated. I guess we can live with that minor duplication though
|
Thanks! |
mcbridejc
changed the title
2 participants
Hello,
I must support u24 in my application.
I started implementing it, but I am not sure it is complete (I do not understand the complete project yet)
Happy to receive directions!
Cheers,