-
Notifications
You must be signed in to change notification settings - Fork 40
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
base: master
Are you sure you want to change the base?
Conversation
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
Any thoughts? |
fb59d71
to
0db3d15
Compare
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. |
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. |
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.
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?). |
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.
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);
} |
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.
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 |
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.