rtlabs-com / c-open

CANopen stack for embedded devices
http://www.rt-labs.com
Other
79 stars 40 forks source link

Support UNSIGNED48 and friends #32

Open nattgris opened 3 years ago

nattgris commented 3 years ago

I've been using these changes locally to fix #31 for a while. There may be a better way that doesn't break the API but I don't see what it would be. Please take a look.

nattgris commented 3 years ago

Any thoughts?

hefloryd commented 3 years ago

In general I think this is fine but was meaning to see if we could add some compile-time check for the unaligned arrays issue. We can document the limitation but it seems like that could be easy to miss and the resulting problem might be hard to track down.

nattgris commented 3 years ago

Maybe the array-supporting code should space the elements only 1, 2, 4 or 8 bytes apart, even if the element size is somewhere in between.

nattgris commented 3 years ago

In general, what happens if the bitlength of an entry doesn't match its datatype (for integers)? If they need to match, why are both specified?

hefloryd commented 3 years ago

In general, what happens if the bitlength of an entry doesn't match its datatype (for integers)? If they need to match, why are both specified?

You mean in co_entry_t? They should match for integers but I don't think it's enforced anywhere. You could look up the bitlength for integers based on the datatype, but for other types such as OCTET_STRING we need to record the size somewhere.

Maybe the array-supporting code should space the elements only 1, 2, 4 or 8 bytes apart, even if the element size is somewhere in between.

That sounds like an interesting idea, especially if we can find a solution that doesn't affect the performance of aligned arrays (which I assume are more common?).

nattgris commented 3 years ago

In general, what happens if the bitlength of an entry doesn't match its datatype (for integers)? If they need to match, why are both specified?

You mean in co_entry_t? They should match for integers but I don't think it's enforced anywhere. You could look up the bitlength for integers based on the datatype, but for other types such as OCTET_STRING we need to record the size somewhere.

If it silently fails if they don't match, I think that's even more likely to need a compile-time check, than the obscure array of odd-sized elements.

Maybe the array-supporting code should space the elements only 1, 2, 4 or 8 bytes apart, even if the element size is somewhere in between.

That sounds like an interesting idea, especially if we can find a solution that doesn't affect the performance of aligned arrays (which I assume are more common?).

Something like this could work, for some reasonably portable/efficient CO_CEILPOW2 (not tested):

diff --git a/src/co_main.h b/src/co_main.h
index dc4de88..0ce4d56 100644
--- a/src/co_main.h
+++ b/src/co_main.h
@@ -59,6 +59,10 @@ extern "C" {

 #define CO_BYTELENGTH(bitlength) (((bitlength) + 7) / 8)

+/* Smallest power of two that is at least n */
+#define CO_CEILPOW2(n) ((n) <= 1 ? 1 : 1UL << (8 * sizeof (unsigned long) - __builtin_clzl(n-1)))
+#define CO_WORDSIZE(bitlength) (CO_CEILPOW2 (CO_BYTELENGTH (bitlength)))
+
 #define CO_RTR_MASK   BIT (30)
 #define CO_EXT_MASK   BIT (29)
 #define CO_ID_MASK    0x1FFFFFFF
diff --git a/src/co_od.c b/src/co_od.c
index 97f5430..4fe2dd9 100644
--- a/src/co_od.c
+++ b/src/co_od.c
@@ -148,7 +148,7 @@ uint32_t co_od_get_ptr (
    else if (entry->flags & OD_ARRAY)
    {
       /* Get pointer to array member */
-      *ptr += (subindex - 1) * CO_BYTELENGTH (entry->bitlength);
+      *ptr += (subindex - 1) * CO_WORDSIZE (entry->bitlength);
    }
    return 0;
 }
@@ -185,7 +185,7 @@ uint32_t co_od_get_value (
    else if (entry->flags & OD_ARRAY)
    {
       /* Get pointer to array member */
-      data += (subindex - 1) * CO_BYTELENGTH (entry->bitlength);
+      data += (subindex - 1) * CO_WORDSIZE (entry->bitlength);
    }

    switch (entry->datatype)
@@ -271,7 +271,7 @@ uint32_t co_od_set_value (

    if (entry->flags & OD_ARRAY)
    {
-      data += (subindex - 1) * CO_BYTELENGTH (entry->bitlength);
+      data += (subindex - 1) * CO_WORDSIZE (entry->bitlength);
    }

    /* Set value in dictionary */
@@ -471,7 +471,7 @@ void co_od_zero (co_net_t * net, uint16_t min, uint16_t max)
                size_t size = CO_BYTELENGTH (entry->bitlength);
                if (entry->flags & OD_ARRAY)
                {
-                  size = size * obj->max_subindex;
+                  size = CO_CEILPOW2 (size) * obj->max_subindex;
                }
                memset (entry->data, 0, size);
             }
hefloryd commented 3 years ago

In general, what happens if the bitlength of an entry doesn't match its datatype (for integers)? If they need to match, why are both specified?

You mean in co_entry_t? They should match for integers but I don't think it's enforced anywhere. You could look up the bitlength for integers based on the datatype, but for other types such as OCTET_STRING we need to record the size somewhere.

If it silently fails if they don't match, I think that's even more likely to need a compile-time check, than the obscure array of odd-sized elements.

True but I think there is a difference between giving an invalid configuration (mismatching bitlength) vs giving a configuration that should be valid if it were not for a limitation of the implementation.

Maybe the array-supporting code should space the elements only 1, 2, 4 or 8 bytes apart, even if the element size is somewhere in between.

That sounds like an interesting idea, especially if we can find a solution that doesn't affect the performance of aligned arrays (which I assume are more common?).

Something like this could work, for some reasonably portable/efficient CO_CEILPOW2 (not tested):

Looks good! The CO_CEILPOW2 macro can be supplied by the porting layer. For our other stacks there is an ..al_sys.h file for stack-specific portability stuff, or if it is generally useful it could go in osal_sys.h