RC_Channel: Express option numbers in bits #19230
Conversation
libraries/RC_Channel/RC_Channel.cpp
Outdated
| @@ -1261,7 +1261,7 @@ RC_Channel *RC_Channels::find_channel_for_option(const RC_Channel::aux_func_t op | |||
| // duplicate_options_exist - returns true if any options are duplicated | |||
| bool RC_Channels::duplicate_options_exist() | |||
| { | |||
| uint8_t auxsw_option_counts[256] = {}; | |||
| uint8_t auxsw_option_counts[308] = {}; | |||
There was a problem hiding this comment.
probably worth adding a 'AUX_FUNC_END` enum for this.
There was a problem hiding this comment.
I agree with this idea. I will change it.
muramura
force-pushed
the
AP_Maximize_the_number_of_duplicate_sequences_for_channel_options
branch
from
937696d
to
aa2655a
Compare
|
@IamPete1 san. I've changed it. |
libraries/RC_Channel/RC_Channel.cpp
Outdated
| @@ -1261,7 +1261,7 @@ RC_Channel *RC_Channels::find_channel_for_option(const RC_Channel::aux_func_t op | |||
| // duplicate_options_exist - returns true if any options are duplicated | |||
| bool RC_Channels::duplicate_options_exist() | |||
| { | |||
| uint8_t auxsw_option_counts[256] = {}; | |||
| uint8_t auxsw_option_counts[uint32_t(RC_Channel::AUX_FUNC::AUX_FUNC_END)] = {}; | |||
There was a problem hiding this comment.
best to change this to a Bitmask object, which would reduce stack usage by 8x
There was a problem hiding this comment.
I change to check with a bit
muramura
force-pushed
the
AP_Maximize_the_number_of_duplicate_sequences_for_channel_options
branch
2 times, most recently
from
28d78f2
to
91e096e
Compare
muramura
changed the title
|
I would change the option number to a bit representation.
|
muramura
force-pushed
the
AP_Maximize_the_number_of_duplicate_sequences_for_channel_options
branch
from
f4690c9
to
f8fc592
Compare
| } | ||
|
|
||
| for (uint16_t i=0; i<sizeof(auxsw_option_counts); i++) { | ||
| if (i == 0) { // MAGIC VALUE! This is AUXSW_DO_NOTHING |
There was a problem hiding this comment.
This lost comment was useful
| @@ -1269,29 +1269,24 @@ RC_Channel *RC_Channels::find_channel_for_option(const RC_Channel::aux_func_t op | |||
| // duplicate_options_exist - returns true if any options are duplicated | |||
| bool RC_Channels::duplicate_options_exist() | |||
| { | |||
| uint8_t auxsw_option_counts[256] = {}; | |||
| uint8_t auxsw_option_counts[uint32_t(RC_Channel::AUX_FUNC::AUX_FUNC_END) / 8 + 1] = {}; | |||
There was a problem hiding this comment.
| uint8_t auxsw_option_counts[uint32_t(RC_Channel::AUX_FUNC::AUX_FUNC_END) / 8 + 1] = {}; | |
| uint8_t auxsw_option_counts[uint32_t((RC_Channel::AUX_FUNC::AUX_FUNC_END) +7 ) / 8] = {}; |
| continue; | ||
| } | ||
| if (auxsw_option_counts[i] > 1) { | ||
| if ((auxsw_option_counts[option / 8] & (1 << (option % 8))) == 0) { |
There was a problem hiding this comment.
It's way too easy to get this wrong. Not saying you have - but I wouldn't trust myself with a quick review of this.
This is why we have the Bitmask class: https://github.com/ardupilot/ardupilot/master/libraries/AP_Common/Bitmask.h#L27
So instead of
uint8_t auxsw_option_counts[uint32_t((RC_Channel::AUX_FUNC::AUX_FUNC_END) +7 ) / 8] = {};
you have
Bitmask<RC_Channel::AUX_FUNC::AUX_FUNC_END> auxsw_option_used;
and then:
| if ((auxsw_option_counts[option / 8] & (1 << (option % 8))) == 0) { | |
| if (auxsw_option_used.get(option)) { |
and then also use .set(option) as appropriate.
|
I've reworked this over here: #27644 Thanks! |
I would change the number of arrays to the maximum value +1 to check for duplication of all channel options.