Skip to content
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

Support UNSIGNED48 and friends #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nattgris
Copy link

@nattgris nattgris commented Dec 3, 2020

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 nattgris marked this pull request as ready for review January 15, 2021 09:56
Andreas Fritiofson added 2 commits September 10, 2021 19:46
Support programmatic access to values larger than 32 bits.

This most straight-forward solution; simply bumping the access
function's value pointer to *uint64_t, unfortunately breaks the API.

Signed-off-by: Andreas Fritiofson <[email protected]>
Change-Id: Id24de582904aeff93f692c46697e1e10f9df3758
Data types such as UNSIGNED48 was not supported, except via the access
function with the previous commit. Add all the non-power-of-two variants
of UNSIGNED and INTEGER to the OD get/set functions.

Note that defining an array of these objects using OD_ARRAY is broken,
because it produces unaligned accesses, that even if they may be
supported by the platform, will not necessarily be atomic.

Signed-off-by: Andreas Fritiofson <[email protected]>
Change-Id: Id7ee68cc50921b9b6705e95a5867b7f99eb319be
@nattgris
Copy link
Author

Any thoughts?

@hefloryd
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UNSIGNED48 is not working
2 participants