Fix incorrect bit check for MaxLRTxPowerLevel in SupportsSerialAPISetup_func#45
Fix incorrect bit check for MaxLRTxPowerLevel in SupportsSerialAPISetup_func#45ptphan28 wants to merge 1 commit into
Conversation
|
Thank you, this looks legit, let me run tests Please update commit message, to explain what was the observed issue and the motivation of the fix (reference to specs will be also appreciated) Reviewers welcome (I cant add names) cc: @lmolina @ochavezmiranda @Thomasdjb @XavierPerraud-Silabs |
|
I noticed an issue (not related to this change) in build log: 1046.6 Generating codsh: 1: mscgen: not found https://en.wikipedia.org/wiki/MscGen May this be fixed |
| @@ -243,9 +243,9 @@ | |||
| "beyond 1st byte of Extended Z-Wave API Setup Supported Sub" | |||
| "Commands bitmask, which is not supported.\n", subcmd); | |||
| } else if (subcmd > 8) { | |||
There was a problem hiding this comment.
I would recommend something closer to the source code of the Z-Wave stack:
uint8_t index = (subcmd - 1) / 8 + 1; // +1 because the sub command bitmask start at index 1 instead of 0.
uint8_t mask = (1 << ((x - 1) % 8));
return capabilities.supported_serialapi_bitmask[index] & mask;
| // check if the subcmd being checked has two bits set for e.g. 3(0011), 5(0101) | ||
| // Where we need to check the Extended Z-Wave API Setup Supported Sub Commands bitmask | ||
| if (subcmd & (subcmd - 1)) { | ||
| if (subcmd > 16) { |
There was a problem hiding this comment.
Why subcmd is limited to 16?
Highest subcmd defined in the controller is 22: SERIAL_API_SETUP_CMD_GET_REGION_INFO.
There was a problem hiding this comment.
I believe the reason is the limitation of the Z/IP Gateway, which only supports a few specific Serial API subcommands.
The Z/IP Gateway supports only the following Serial API subcommands:
SERIAL_API_SETUP_CMD_UNSUPPORTED,
SERIAL_API_SETUP_CMD_SUPPORTED = 1<<0,
SERIAL_API_SETUP_CMD_TX_STATUS_REPORT = 1<<1,
SERIAL_API_SETUP_CMD_MAX_LR_TX_PWR_SET = 3,
SERIAL_API_SETUP_CMD_TX_POWERLEVEL_SET = 1<<2,
SERIAL_API_SETUP_CMD_MAX_LR_TX_PWR_GET = 5,
SERIAL_API_SETUP_CMD_TX_POWERLEVEL_GET = 1<<3,
// SERIAL_API_SETUP_CMD_TX_GET_MAX_PAYLOAD_SIZE = 1<<4,
SERIAL_API_SETUP_CMD_RF_REGION_GET = 1<<5,
SERIAL_API_SETUP_CMD_RF_REGION_SET = 1<<6,
SERIAL_API_SETUP_CMD_NODEID_BASETYPE_SET = 1<<7,
|
Can this change be merged in? We needed this change when using the latest zipgateway. FYI @lmolina |
|
@ptphan28 would you add unit test to cover the change? Also, could you develop the commit message? I suggest some like:
If the refactor in #45 (comment) is a blocker, I suggest to do it in a separate commit and tag it as refactor. Balance that the project is in maintenance. |
| return (is_bit_num_set_in_byte(subcmd - 9, capabilities.supported_serialapi_bitmask[2])); | ||
| } else if (subcmd > 0) { | ||
| return (is_bit_num_set_in_byte(subcmd ,capabilities.supported_serialapi_bitmask[1])); | ||
| return (is_bit_num_set_in_byte(subcmd - 1,capabilities.supported_serialapi_bitmask[1])); |
There was a problem hiding this comment.
| return (is_bit_num_set_in_byte(subcmd - 1,capabilities.supported_serialapi_bitmask[1])); | |
| return (is_bit_num_set_in_byte(subcmd - 1, capabilities.supported_serialapi_bitmask[1])); |
| @@ -243,9 +243,9 @@ BYTE SupportsSerialAPISetup_func(BYTE subcmd) | |||
| "beyond 1st byte of Extended Z-Wave API Setup Supported Sub" | |||
| "Commands bitmask, which is not supported.\n", subcmd); | |||
| } else if (subcmd > 8) { | |||
5 participants
This PR fixes an issue in SupportsSerialAPISetup_func() where the subcommand ID was incorrectly used as a bit index in is_bit_num_set_in_byte(). This prevented MaxLRTxPowerLevel from being configured via zipgateway.cfg.