Reading 8- or 16-bit attributes from big-endian device returns 0 (#6166)

[pjzander-signify]

Problem

When reading an 8- or 16-bit attribute from a big endian device the value is 0, despite that defaults other than 0 are defined.

The defaults of these attributes are defined in a zap-file and from that a header file is generated (endpoint_config.h).
This header file contains structures that are used by the software to read cluster information and the attributes of these clusters.
The value of these attributes are stored in a union that also contains 2 pointers:
typedef union { uint8_t * ptrToDefaultValue; uint16_t defaultValue; EmberAfAttributeMinMaxValue * ptrToMinMaxValue; } EmberAfDefaultOrMinMaxAttributeValue;

A generated default value is compiled into that union using an explicit type cast to a pointer.
Example:
{ (uint8_t *) 3 } }
In a 32-bit little-endian the memory layout will be:
03 00 00 00
The 16-bit defaultValue (the first 2 bytes) will have the value 3, which is correct.

In a 32-bit big-endian system the layout will be:
00 00 00 03
The 16-bit defaultValue (the first 2 bytes) will have the value 0, which is not correct.

Summary of Changes

Use a structure for the defaultValue, and that structure has the same size as a pointer.
Then use alignment bits to always align the 16-bit defaultValue with the least-significant-bits of the pointer.

NOTE:
Tested it on:
32-bit little-endian (esp32)
64-bit little-endian (linux)
32-bit big-endian (openwrt)

Fixes #6166

[mspang]

This seems to point to deeper issues.

e.g., why is the default being stored in 16-bit field and read out as an 8-bit one?

[bzbarsky-apple]

This seems to be making assumptions about how a bitfield and an integer will pack that might not be valid in practice.... Nothing I'm aware of requires them to pack with no gaps here.

At the very least we should static_assert that sizeof(EmberAfAlignValue16) == sizeof(uint8_t*), right? That way we will catch it if things pack wrong.

[pjzander-signify]

This seems to be making assumptions about how a bitfield and an integer will pack that might not be valid in practice.... Nothing I'm aware of requires them to pack with no gaps here.
You are right on that one.

[bzbarsky-apple]

Why this change to the comment? The new comment does not seem to be correct if the attribute type is larger than 2-byte; in that case this field is not really the default value in any sense; the ptrToDefaultValue points to where the default value is.

[bzbarsky-apple]

Thinking about this some more: it seems like the current setup is more or less a holdover from all this code being C (though why it did not use named-union-member initialization, I don't know).

In C++ we should be able to define the union like so (stripping out comments for brevity):

union EmberAfDefaultAttributeValue
{
    constexpr EmberAfDefaultAttributeValue(uint8_t * ptr) : ptrToDefaultValue(ptr) {}
    constexpr EmberAfDefaultAttributeValue(uint16_t val) : defaultValue(val) {}

    uint8_t * ptrToDefaultValue;
    uint16_t defaultValue;
};

and change the codegen to emit a cast to uint16_t, not uint8_t*, for default values that should be treated as integers. I'd expect that should make things work right.... (I tried having no cast at all, but some of the default values are 0, and those end up ambiguous between 0-valued integer and null pointer without a disambiguating something.)

[bzbarsky-apple]

e.g., why is the default being stored in 16-bit field and read out as an 8-bit one?

It's read as a 16-bit or 8-bit field depending on what the declared size of the attr is.

We could have separate union fields for the 16-bit and 8-bit cases, yes. And remove the big-endian-only "move forward by a byte" logic for the 8-bit case. But at least that logic is correct, if the value is correctly placed in the 16-bit field....

[mspang]

This code seems to be intended to account for this:

connectedhomeip/src/app/util/attribute-storage.cpp

Lines 1112 to 1121 in fca3657

	#if (BIGENDIAN_CPU)
	// The default value for one- and two-byte attributes is stored in an
	// uint16_t. On big-endian platforms, a pointer to the default value of
	// a one-byte attribute will point to the wrong byte. So, for those
	// cases, nudge the pointer forward so it points to the correct byte.
	if (emberAfAttributeSize(am) == 1 && ptr != NULL)
	{
	*ptr++;
	}
	#endif // BIGENDIAN

[bzbarsky-apple]

This code seems to be intended to account for this:

Right, that's the code that handles 8-bit vs 16-bit.

I did check with @selissia and he confirmed that the overall issue here is in fact a problem. @pjzander-signify thoughts on #6175 (review) ?

[pjzander-signify]

I agree with Boris Zbarsky on his remark about how a bitfield and an integer will be packed.

The problem arises when reading the generated default and min/max values in a big endian system.
So another option is that the generated file endpoint_config.h will contain a little endian and a big endian part on these value.
For example:

#if BIGENDIAN_CPU
#if (UINTPTR_MAX == UINT32_MAX)
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 0x00030000 } }, \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x08000000 } },        \
  ...

#elif (UINTPTR_MAX == UINT64_MAX)
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 0x0003000000000000 } }, \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x0800000000000000 } },  \
  ...

#endif

#define GENERATED_ATTRIBUTES { \

#else // !BIGENDIAN_CPU
#define GENERATED_DEFAULTS { \
#define GENERATED_ATTRIBUTES { \
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 3 } },   \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x08 } }, \
  ...

#endif // BIGENDIAN_CPU

@bzbarsky-apple what do you think of this solution?

[bzbarsky-apple]

@bzbarsky-apple what do you think of this solution?

@pjzander-signify We can do that. It just seems like a lot more work than #6175 (review) to me....

[pjzander-signify]

@bzbarsky-apple what do you think of this solution?

@pjzander-signify We can do that. It just seems like a lot more work than #6175 (review) to me....

If you agree I will fix it this way.

[bzbarsky-apple]

This way being with the ifdefs in the zap output? Again, it seems more complex and more fragile, and much harder to read the result; are there reasons to prefer it?

[pjzander-signify]

@pjzander-signify We can do that. It just seems like a lot more work than #6175 (review) to me....

I created the solution as I described (generating big-endian defaults).
For this I also had to change a file (helper-endpointconfig.js) that is in the ZAP-repo.
@bzbarsky-apple: How do I handle this? Create a separate ticket for the ZAP-repo? And do a pull request on that repo?

[pjzander-signify]

This way being with the ifdefs in the zap output? Again, it seems more complex and more fragile, and much harder to read the result; are there reasons to prefer it?

I do not agree on this. The changes are quite simple and there is no risk to break little endian builds.

[bzbarsky-apple]

@bzbarsky-apple: How do I handle this? Create a separate ticket for the ZAP-repo? And do a pull request on that repo?

Yes. If you can reference that pull request here, that would be much appreciated.

[bzbarsky-apple]

But I should note that that solution is also making assumptions that are not guaranteed to hold. For example, it's assuming that if UINTPTR_MAX == UINT64_MAX that means that pointers are 8-byte, but that's not necessarily the case. The C++ standard allows uintptr_t to be 8-byte when pointers are 4-byte (or 2-byte, or 6-byte, or whatever).... This is a more theoretical concern than how bitfields pack, but it does involve us depending on behavior that is not technically guaranteed by the standard.

[bzbarsky-apple]

@bzbarsky-apple: How do I handle this? Create a separate ticket for the ZAP-repo? And do a pull request on that repo?

Actually, I'd think this would just be handled by changes to src/app/zap-templates/templates/app/endpoint_config.zapt in the CHIP repo....

[vivien-apple]

The problem arises when reading the generated default and min/max values in a big endian system.
So another option is that the generated file endpoint_config.h will contain a little endian and a big endian part on these value.
For example:

#if BIGENDIAN_CPU
#if (UINTPTR_MAX == UINT32_MAX)
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 0x00030000 } }, \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x08000000 } },        \
  ...

#elif (UINTPTR_MAX == UINT64_MAX)
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 0x0003000000000000 } }, \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x0800000000000000 } },  \
  ...

#endif

#define GENERATED_ATTRIBUTES { \

#else // !BIGENDIAN_CPU
#define GENERATED_DEFAULTS { \
#define GENERATED_ATTRIBUTES { \
  { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, { (uint8_t *) 3 } },   \
  { 0x0000, ZAP_TYPE(INT8U), 1, 0, { (uint8_t *) 0x08 } }, \
  ...

#endif // BIGENDIAN_CPU

I would rather prefer to use what Boris has suggested:

-typedef union
+union EmberAfDefaultAttributeValue
 {
+    constexpr EmberAfDefaultAttributeValue(uint8_t * ptr) : ptrToDefaultValue(ptr) {}
+    constexpr EmberAfDefaultAttributeValue(uint16_t val) : defaultValue(val) {}
+
     /**
      * Points to data if size is more than 2 bytes.
      * If size is more than 2 bytes, and this value is NULL,
      * then the default value is all zeroes.
      */
     uint8_t * ptrToDefaultValue;
+
     /**
      * Actual default value if the attribute size is 2 bytes or less.
      */
     uint16_t defaultValue;
-} EmberAfDefaultAttributeValue;
+};
...
-typedef union
+union EmberAfDefaultOrMinMaxAttributeValue
 {
+    constexpr EmberAfDefaultOrMinMaxAttributeValue(uint8_t * ptr) : ptrToDefaultValue(ptr) {}
+    constexpr EmberAfDefaultOrMinMaxAttributeValue(uint16_t val) : defaultValue(val) {}
+
     /**
      * Points to data if size is more than 2 bytes.
      * If size is more than 2 bytes, and this value is NULL,
      * then the default value is all zeroes.
      */
     uint8_t * ptrToDefaultValue;
     /**
      * Actual default value if the attribute size is 2 bytes or less.
      */
     uint16_t defaultValue;
     /**
      * Points to the min max attribute value structure, if min/max is
      * supported for this attribute.
      */
     EmberAfAttributeMinMaxValue * ptrToMinMaxValue;
-} EmberAfDefaultOrMinMaxAttributeValue;
+};

and update endpoint_config.zapt template with:

 #define ZAP_LONG_DEFAULTS_INDEX(index)                                                                                             \
     {                                                                                                                              \
         (uint8_t *) (&generatedDefaults[index])                                                                                    \
     }
 #define ZAP_MIN_MAX_DEFAULTS_INDEX(index)                                                                                          \
     {                                                                                                                              \
         (uint8_t *) (&minMaxDefault[index])                                                                                        \
     }
 #define ZAP_EMPTY_DEFAULT()                                                                                                        \
     {                                                                                                                              \
-        (uint8_t *) 0                                                                                                              \
+        (uint16_t) 0                                                                                                               \
     }
 #define ZAP_SIMPLE_DEFAULT(x)                                                                                                      \
     {                                                                                                                              \
-        (uint8_t *) x                                                                                                              \
+        (uint16_t) x                                                                                                               \
     }

It makes it very clear from the template what is the destination type for the values.

[pjzander-signify]

@bzbarsky-apple: How do I handle this? Create a separate ticket for the ZAP-repo? And do a pull request on that repo?

Actually, I'd think this would just be handled by changes to src/app/zap-templates/templates/app/endpoint_config.zapt in the CHIP repo....

Indeed, you need an update of endpoint_config.zapt but also in helper-endpointconfig.js because the function endpoint_attribute_list does not support big-endain generation.

[pjzander-signify]

I would rather prefer to use what Boris has suggested

This does not solve the big-endian issue.

[bzbarsky-apple]

This does not solve the big-endian issue.

As in you tested that approach and the bytes still end up in the wrong place in the union?

[pjzander-signify]

As in you tested that approach and the bytes still end up in the wrong place in the union?

With the solution I propose the bytes are properly aligned.
The only modifications are in the zap-submodule (helper-endpointconfig.js) and in endpoint_config.zapt.
For the zap-submodule I created a ticket and did a pull request (project-chip/zap#142).

The generated output looks like this:

#if BIGENDIAN_CPU
// 32-bits big-endian:
#if (UINTPTR_MAX == UINT32_MAX)
#define GENERATED_ATTRIBUTES                                                                                                       \
    {                                                                                                                              \
        ...
        { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, ZAP_SIMPLE_DEFAULT(0x00010000) }, /* cluster revision */                             \
        ...
#endif
#else // !BIGENDIAN_CPU
// little-endian:
#define GENERATED_ATTRIBUTES                                                                                                       \
    {                                                                                                                              \
        ...
            { 0xFFFD, ZAP_TYPE(INT16U), 2, 0, ZAP_SIMPLE_DEFAULT(0x0001) },    /* cluster revision */                              \
        ...
#endif // BIGENDIAN_CPU

No modifications needed in af-types.h, attribute-storage.cpp and attribute-table.cpp.

[pjzander-signify]

I will create a new pull request

[bzbarsky-apple]

With the solution I propose the bytes are properly aligned.

That wasn't my question.... I understand that your solution works. I was asking whether you are claiming that my proposed solution does not work.

[pjzander-signify]

That wasn't my question.... I understand that your solution works. I was asking whether you are claiming that my proposed solution does not work.

Sorry @bzbarsky-apple , I totally misunderstood your question.
I tested your solution on a MIPS system and it works!
I will create a new pull request with your solution.