Added an initial attempt of having the IM support (U)INT24 types #11256
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
tlykkeberg-grundfos
force-pushed
the
feature/extend-im-with-uint24-support
branch
from
b0e3145 to
3d38e00
Compare
|
PR #11256: Size comparison from 3947eba to 7bfb306 Increases (7 builds for linux, mbed, p6)
Decreases (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (37 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
There was a problem hiding this comment.
I think it would be better to not make any sort of changes to the TLV bits at all and instead do all the conversion as needed inside ember-compatibility-functions and Accessors.h/cpp. That would allow us to reasonably represent int24 as int32_t in most code, with only the code that needs to worry about the attribute store needing to deal with the actually-narrower range and 3-byte representation....
| case ZCL_INT24U_ATTRIBUTE_TYPE: // Unsigned 24-bit integer | ||
| { | ||
| TLV::uint24_t uint24_data; | ||
| memcpy(static_cast<void *>(&uint24_data), attributeData, sizeof(uint24_data)); |
There was a problem hiding this comment.
Have we double-checked that the actual space in the attribute store that is allocated for this type is 3 bytes?
There was a problem hiding this comment.
I have found the default size in the array GENERATED_DEFAULTS in the autogenerated file: endpoint_config.h to be 3 bytes for both the Power and the LifetimeRunningHours attributes, which are both UINT24.
/* Endpoint: 1, Cluster: Pump Configuration and Control (server), big-endian */ \
\
/* 2351 - LifetimeRunningHours, */ \
0x00, 0x00, 0x00, \
\
/* 2354 - Power, */ \
0x00, 0x00, 0x00, \
And in the same file, I found the Attribute size, in the GENERATED_ATTRIBUTES array, to be 3 for those attributes as well.
{ 0x0015, ZAP_TYPE(INT24U), 3, ZAP_ATTRIBUTE_MASK(WRITABLE), \
ZAP_LONG_DEFAULTS_INDEX(2351) }, /* LifetimeRunningHours */ \
{ 0x0016, ZAP_TYPE(INT24U), 3, ZAP_ATTRIBUTE_MASK(WRITABLE), ZAP_LONG_DEFAULTS_INDEX(2354) }, /* Power */ \
There was a problem hiding this comment.
And if these values are the foundation for the attribute store, then I take it that they are actually 3 bytes long. But perhaps I'm not looking the correct places?
|
I think you are right Boris. The changes should only go into the |
|
But looking into the I took a look at the Here we notice, that the autogenrated code produces a ZCL type specific call to the There is no issue when trying to read a 24bit attribute using a 32bit value, since the length of the |
…ntroller-clusters.zap` and the `all-clusters-app.zap` files
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
carol-apple,
cecille and
chrisdecenzo
pullapprove
bot
requested review from
mlepage-google,
mrjerryjohns,
msandstedt,
mspang,
pan-apple,
sagar-apple,
saurabhst,
selissia,
tecimovic,
vivien-apple,
wbschiller,
woody-apple and
yunhanw-google
|
PR #11256: Size comparison from 787b7ec to b9a2a5f Increases above 0.2%:
Increases (29 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (37 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
Given that this PR is in progress maybe it should be better to change it to draft |
tlykkeberg-grundfos
changed the title
|
PR #11256: Size comparison from 4947759 to d241b5f Increases above 0.2%:
Increases (30 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (1 build for p6)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
requested changes
Nov 4, 2021
| case ZCL_INT24U_ATTRIBUTE_TYPE: // Unsigned 24-bit integer | ||
| { | ||
| uint32_t uint32_data; | ||
| uint32_data = emberAfGetInt24u(attributeData, 0, 3); |
There was a problem hiding this comment.
So this is assuming attributeData is little-endian, not native-endian, right? Is that a correct assumption for the way the values are stored, especially the default values?
There was a problem hiding this comment.
This is only copying 3 bytes which are to be in the machine endianness (little or big). I believe that the attribute-data in the attribute store is always stored in machine (CPU) endianness.
There was a problem hiding this comment.
emberAfGetInt24u always reads little-endian data, no?
Comment on lines
+295
to
+297
| // Trigger the sign extension | ||
| int32_data <<= 8; | ||
| int32_data >>= 8; |
There was a problem hiding this comment.
Shouldn't that happen inside emberAfGetInt24s? It sounds like that's not implemented correctly....
And again, the endianness question.
There was a problem hiding this comment.
Yes, I think it should happen inside the method, but it does not - and I was currently forced to use this special construction "to grab the sign". Perhaps it should go into the emberAfGetUnt24s() method is self?
There was a problem hiding this comment.
Ideally yes. Followup for that is fine. But in general, I suspect we don't want to use emberAfGetUnt24s here anyway because of the endianness problem: it always reads little-endian data.
| return CHIP_ERROR_INVALID_INTEGER_VALUE; | ||
| } | ||
| dataLen = emberAfGetDataSize(baseType); | ||
| emberAfCopyInt24u(attributeData, 0, value); |
There was a problem hiding this comment.
OK, so this is in fact storing as little-endian, so I guess the question is just around default values.
Well, that and the oddity of storing these as little-endian while all other integers are stored native-endian...
| int32_t value; | ||
| static_assert(3 <= sizeof(attributeData), "Value cannot fit into attribute data"); | ||
| ReturnErrorOnFailure(aReader.Get(value)); | ||
| if (value > static_cast<int32_t>((1UL << ((3 * 8) - 1)) - 1) || value < static_cast<int32_t>(-(1UL << ((3 * 8) - 1)))) |
There was a problem hiding this comment.
Suggested change
| if (value > static_cast<int32_t>((1UL << ((3 * 8) - 1)) - 1) || value < static_cast<int32_t>(-(1UL << ((3 * 8) - 1)))) | |
| if (value > static_cast<int32_t>((1UL << ((3 * 8) - 1)) - 1) || value < static_cast<int32_t>(-(1UL << ((3 * 8) - 1) ) + 1)) |
The most-negative value representable in 24 bits is not an allowed value, right?
There was a problem hiding this comment.
The 2. complement values of a 24-bit signed value are:
Which is -8388608 to 8388607
Just like the 8-bit version is:
At least to the best of my knowlegde. So the expression is all wrong either way ;-)
There was a problem hiding this comment.
The point is, -2^(24-1) is not a valid uint24 value in the spec.
| @@ -595,20 +595,26 @@ | |||
| \ | |||
| /* Endpoint: 1, Cluster: Pump Configuration and Control (server), big-endian */ \ | |||
| \ | |||
| /* 4574 - LifetimeEnergyConsumed, */ \ | |||
| /* 4574 - LifetimeRunningHours, */ \ | |||
| 0x00, 0x00, 0x00, \ | |||
There was a problem hiding this comment.
So the good news is that the default is in fact 3 bytes. If the default were a value where we could see the byte ordering, that would be helpful in terms of seeing whether reading as always-little-endian makes sense.
There was a problem hiding this comment.
I'll try to make a test with other default values.
There was a problem hiding this comment.
There are two DEFAULT tables in the endpoint_config.h fiel. One for big-endian CPU's and one for little-endian CPU's. So I think this is taken care of when compiling. So the typeSensitiveMemCopy() will work correctly, since all data are stored locally on the device according to it's endianess - at least that is what I can conclude from the code.
There was a problem hiding this comment.
But the zap tool does not generate it correctly. I can see the following in the endpoint_config.h file after setting the LifetimeRunningHours default value to 0x123456 in the zap tool UI and regenrating the files using ./scripts/tools/zap_regen_all.py.
/* Endpoint: 1, Cluster: Pump Configuration and Control (server), big-endian */ \
\
/* 1377 - LifetimeRunningHours, */ \
0x12, 0x34, 0x56, \and
/* Endpoint: 1, Cluster: Pump Configuration and Control (server), little-endian */ \
\
/* 1377 - LifetimeRunningHours, */ \
0x12, 0x34, 0x56, \This is not expected, since these are encapsulated in an if-else:
// Default values for the attributes longer than a pointer,
// in a form of a binary blob
// Separate block is generated for big-endian and little-endian cases.
#if BIGENDIAN_CPU
#define GENERATED_DEFAULTS \
{
......
}
#else // !BIGENDIAN_CPU
#define GENERATED_DEFAULTS \
{ \
......
}
#endif // BIGENDIAN_CPU
``There was a problem hiding this comment.
Should be fixed now, with #11451 merged.
Comment on lines
+979
to
+980
| return emberAfReadServerAttribute(endpoint, Clusters::DeviceTemperatureConfiguration::Id, Id, (uint8_t *) value, | ||
| sizeof(*value)); |
There was a problem hiding this comment.
This is not going to do the right thing.... It's reading as native-endian, and reading 4 bytes, not 3. We clearly need better tests here somehow.
There was a problem hiding this comment.
You might be right. The call to emberAfReadServerAttribute() ends up in the typeSensitiveMemCopy() method, where the argument readLength is the value passed in the call to emberAfReadServerAttribute as the sizeof(*value). In the case of INT24U this value is 3. But in the actual memmove() we see this:
if (!ignoreReadLength && readLength < am->size)
{
return EMBER_ZCL_STATUS_INSUFFICIENT_SPACE;
}
if (src == NULL)
{
memset(dest, 0, am->size);
}
else
{
memmove(dest, src, am->size);
}
This is the part of the code which we end up, when handling these types, which is not Strings, LongStrings or Lists.
The readLength is not used in this case, but just the attribute size it self am->size
There was a problem hiding this comment.
Hmm. OK, that is a good point: the callee will only copy 3 bytes, and put them into our 4-byte space... where? Unless there's some endianness-checking, it will put them in the "first 3 bytes", which will behave correctly for little-endian (but only if we make sure to zero out the uint32_t first) and not behave correctly for big-endian....
It might be clearer to read into an on-stack 3-byte buffer and then build the 32-bit int from that...
Comment on lines
+984
to
+985
| return emberAfWriteServerAttribute(endpoint, Clusters::DeviceTemperatureConfiguration::Id, Id, (uint8_t *) &value, | ||
| ZCL_INT24U_ATTRIBUTE_TYPE); |
There was a problem hiding this comment.
And this is writing native-endian, and the first 3 bytes, which might be the three high bytes or the three low bytes, depending on endianness.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Change overview
ZCL_INT24U_andZCL_INT24S_types in theember-compatibility-functions.cppmoduleINT24UandINT24Stypes in theAccessory.jshelper JavaScriptPowerand theLifetimeRunningHours24-bit attributes in the PCC cluster in thecontroller-clusters.zapand theall-clusters-app.zapfilesTesting
./scripts/tools/zap_regen_all.pychip-toolandall-clusters-appand tested on Darwin platform