bi_pin_mask_with_names assumes order #878

New issue

Closed

opened 2022-06-21 09:49:00 -05:00 by ecrips · 1 comment

ecrips commented

2022-06-21 09:49:00 -05:00

(Migrated from github.com)

The SDK provides 3 functions of the form bi_2pins_with_names which allow specifying multiple pins and their names. However, because they are implemented in terms of bi_pin_mask_with_names which takes a single bitmask, there is an (undocumented!) assumption that the pins are passed in ascending numeric order.

For example:

bi_decl_if_func_used(bi_2pins_with_names( 5, "Pin 5", 4, "Pin 4" ))

produces the following output from picotool:

Fixed Pin Information
 4:   Pin 5
 5:   Pin 4

Swapping the order of the argument fixes it to behave as expected. Obviously the GPIO numbers might not be hardcoded like that but may come from e.g. a configuration option.

A patch like the following at least triggers a build error, even better if someone can work out a way to rearrange the order automatically.

diff --git a/src/common/pico_binary_info/include/pico/binary_info/code.h b/src/common/pico_binary_info/include/pico/binary_info/code.h
index e87a2cd..7eb75d9 100644
--- a/src/common/pico_binary_info/include/pico/binary_info/code.h
+++ b/src/common/pico_binary_info/include/pico/binary_info/code.h
@@ -134,8 +134,13 @@ static const struct _binary_info_named_group __bi_lineno_var_name = { \
 // names are sperated by | ... i.e. "name1|name2|name3"
 #define bi_pin_mask_with_names(pmask, label)          __bi_pins_with_name((pmask), (label))
 #define bi_1pin_with_name(p0, name)                   bi_pin_mask_with_name(1u << (p0), name)
-#define bi_2pins_with_names(p0, name0, p1, name1)     bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)), name0 "|" name1)
-#define bi_3pins_with_names(p0, name0, p1, name1, p2, name2)  bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)) | (1u << (p2)), name0 "|" name1 "|" name2)
-#define bi_4pins_with_names(p0, name0, p1, name1, p2, name2, p3, name3)  bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)) | (1u << (p2)) | (1u << (p3)), name0 "|" name1 "|" name2 "|" name3)
+#define bi_combine_pins(p0, p1)                       ({static_assert(p0 < p1, "Pin numbers must be in order"); p0 | p1; })
+#define bi_2pins_with_names(p0, name0, p1, name1)     bi_pin_mask_with_names(bi_combine_pins(1u << (p0), 1u << (p1)), name0 "|" name1)
+#define bi_3pins_with_names(p0, name0, p1, name1, p2, name2)  bi_pin_mask_with_names(bi_combine_pins(1u << (p0),                                          \
+                                                                                                     bi_combine_pins(1u << (p1), 1u << (p2))),            \
+                                                                                                     name0 "|" name1 "|" name2)
+#define bi_4pins_with_names(p0, name0, p1, name1, p2, name2, p3, name3)  bi_pin_mask_with_names(bi_combine_pins(bi_combine_pins(1u << (p0), 1u << (p1)),  \
+                                                                                                                bi_combine_pins(1u << (p2), 1u << (p3))), \
+                                                                                                name0 "|" name1 "|" name2 "|" name3)
 
 #endif

The SDK provides 3 functions of the form `bi_2pins_with_names` which allow specifying multiple pins and their names. However, because they are implemented in terms of `bi_pin_mask_with_names` which takes a single bitmask, there is an (undocumented!) assumption that the pins are passed in ascending numeric order. For example: ``` bi_decl_if_func_used(bi_2pins_with_names( 5, "Pin 5", 4, "Pin 4" )) ``` produces the following output from `picotool`: ``` Fixed Pin Information 4: Pin 5 5: Pin 4 ``` Swapping the order of the argument fixes it to behave as expected. Obviously the GPIO numbers might not be hardcoded like that but may come from e.g. a configuration option. A patch like the following at least triggers a build error, even better if someone can work out a way to rearrange the order automatically. ``` diff --git a/src/common/pico_binary_info/include/pico/binary_info/code.h b/src/common/pico_binary_info/include/pico/binary_info/code.h index e87a2cd..7eb75d9 100644 --- a/src/common/pico_binary_info/include/pico/binary_info/code.h +++ b/src/common/pico_binary_info/include/pico/binary_info/code.h @@ -134,8 +134,13 @@ static const struct _binary_info_named_group __bi_lineno_var_name = { \ // names are sperated by | ... i.e. "name1|name2|name3" #define bi_pin_mask_with_names(pmask, label) __bi_pins_with_name((pmask), (label)) #define bi_1pin_with_name(p0, name) bi_pin_mask_with_name(1u << (p0), name) -#define bi_2pins_with_names(p0, name0, p1, name1) bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)), name0 "|" name1) -#define bi_3pins_with_names(p0, name0, p1, name1, p2, name2) bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)) | (1u << (p2)), name0 "|" name1 "|" name2) -#define bi_4pins_with_names(p0, name0, p1, name1, p2, name2, p3, name3) bi_pin_mask_with_names((1u << (p0)) | (1u << (p1)) | (1u << (p2)) | (1u << (p3)), name0 "|" name1 "|" name2 "|" name3) +#define bi_combine_pins(p0, p1) ({static_assert(p0 < p1, "Pin numbers must be in order"); p0 | p1; }) +#define bi_2pins_with_names(p0, name0, p1, name1) bi_pin_mask_with_names(bi_combine_pins(1u << (p0), 1u << (p1)), name0 "|" name1) +#define bi_3pins_with_names(p0, name0, p1, name1, p2, name2) bi_pin_mask_with_names(bi_combine_pins(1u << (p0), \ + bi_combine_pins(1u << (p1), 1u << (p2))), \ + name0 "|" name1 "|" name2) +#define bi_4pins_with_names(p0, name0, p1, name1, p2, name2, p3, name3) bi_pin_mask_with_names(bi_combine_pins(bi_combine_pins(1u << (p0), 1u << (p1)), \ + bi_combine_pins(1u << (p2), 1u << (p3))), \ + name0 "|" name1 "|" name2 "|" name3) #endif ```

Rows
Columns