Add BTS Authorization Logic and 7-Segment LED Status Display #216
Conversation
.envrc
Outdated
| export SCONSFLAGS="${SCONSFLAGS:-} --directory=$(expand_path .)" | ||
|
|
||
| export SCONSFLAGS="${SCONSFLAGS:-} --directory=$(expand_path .)" | ||
|
|
||
| export SCONSFLAGS="${SCONSFLAGS:-} --directory=$(expand_path .)" | ||
|
|
||
| export SCONSFLAGS="${SCONSFLAGS:-} --directory=$(expand_path .)" | ||
|
|
||
| export SCONSFLAGS="${SCONSFLAGS:-} --directory=$(expand_path .)" | ||
|
|
There was a problem hiding this comment.
What was the point of doing this?
components/sup/src/sup.c
Outdated
| SEG_A = 13, | ||
| SEG_B = 4, | ||
| SEG_C = 7, | ||
| SEG_D = 8, | ||
| SEG_E = 9, | ||
| SEG_F = 12, | ||
| SEG_G = 5, | ||
| SEG_DP = 11, | ||
| SEG_1 = 1, | ||
| SEG_2 = 2, | ||
| SEG_3 = 3, | ||
| SEG_4 = 6, | ||
| } SEVEN_SEG_PINS; |
There was a problem hiding this comment.
ESP-IDF defines macros for pin values, try to use those over numeric values instead, even if in this case they evaluate to the same thing. Since, C lacks namespaces, please prefix enums with some kind of common value like PIN_. Also, try to follow the convention for typedefs of name_t, in this case I'd call it seven_segment_pin_t, however, since this is only in the scope of sup.c, it is safe to just make this an anonymous enum. You also include the indices for the digit selection, but give them a name that's easy to interpret as a segment. It's also strange that you start indexing at 1, does the datasheet do the same? I'd also personally turn this into a lookup table for the digit index so that you could write something like PIN_DIGIT[N] and have a pragmatic way to select the digit you want.
There was a problem hiding this comment.
On second thought, all pins should be in this enum, or just be #defines. The lookup tables can be like your zero and one array.
components/sup/src/sup.c
Outdated
| static enum { | ||
| bbc_state, | ||
| throttle_state, | ||
| steer_state | ||
| } current_state = bbc_state; |
There was a problem hiding this comment.
enum constants should be in SNAKE_CASE.
components/sup/src/sup.c
Outdated
| steer_state | ||
| } current_state = bbc_state; | ||
|
|
||
| static void init_pin(SEVEN_SEG_PINS pin) |
There was a problem hiding this comment.
I see the vision, but this is common use-case, so look into using gpio_config_t to achieve similar behavior.
components/sup/src/sup.c
Outdated
| static void set_one() | ||
| { | ||
| for (int i = 0; i < 6; i++) { | ||
| gpio_set_level(zero[i], 1); | ||
| } | ||
| for (int i = 0; i < 2; i++) { | ||
| gpio_set_level(one[i], 0); | ||
| } | ||
| gpio_set_level(SEG_G, 1); | ||
| } | ||
|
|
||
| static void set_zero() | ||
| { | ||
| for (int i = 0; i < 6; i++) { | ||
| gpio_set_level(zero[i], 0); // active low | ||
| } | ||
| gpio_set_level(SEG_G, 1); | ||
| } |
There was a problem hiding this comment.
Once, again, I see the vision, but you can make this a general function that accepts a bitfield as an argument, since the seven segments + decimal point can be represented as 8-bits. Instead make a function that maps each bit of a byte into something you can display. For special digit values you can define a constant (i.e write_segments(SEGMENTS_PATTERN_1)).
components/sup/src/sup.c
Outdated
| gpio_set_level(SEG_2, 0); | ||
| gpio_set_level(SEG_3, 1); | ||
| } | ||
| current_state = (current_state + 1) % 3; |
There was a problem hiding this comment.
No magic numbers! Add an additional member to your state enum called STATES_TOTAL. Since enums start counting at zero, the last element will conveniently have the same value as the number of states.
Implements logic for BTS (Brake, Throttle, Steer) authorization checks and visualizes the authorization status using a 7-segment display. It cycles through each component (BBC, Throttle, Steer) at 1kHz, displaying 0 if authorized and 1 if not.