Fola/led class #11
Fola/led class #11
Conversation
| @@ -2,14 +2,24 @@ | |||
| * ws2812.h | |||
| * | |||
| * Created on: Sep 22, 2024 | |||
| * Author: anni and Fola | |||
| * Based on | |||
| * Author: anni | |||
There was a problem hiding this comment.
You forgot to add yourself!!!!
| * Author: anni | |
| * Author: Anni, Fola |
There was a problem hiding this comment.
Overall great work!
I saw these working IRL, so I'll approve the PR, but please attach photos/videos as well as a short description detaliing your testing and showing the LED's working.
This way anyone else that wasn't there reading the PR can understand the state that the LED system is left in just by reading your PR description.
| #define PWM_HI (38) | ||
| #define PWM_LO (19) |
There was a problem hiding this comment.
A brief explanation explaining why these constants are the way they are would be mega appreciated.
Is this number of counter counts? % value? etc...
| #define BUFF_SIZE (NUM_LEDS + 2) * 24 | ||
|
|
||
| extern uint8_t out_buf[BUFF_SIZE]; //Byte buffer of values to be transferred to the capture compare register (CCR). | ||
| #define BUFF_SIZE (NUM_LEDS + 2) * 24 // We add 24 to the end so that we have 24 (one LED) bits of 0 |
There was a problem hiding this comment.
I would wrap BUFF_SIZE in another set of brackets:
| #define BUFF_SIZE (NUM_LEDS + 2) * 24 // We add 24 to the end so that we have 24 (one LED) bits of 0 | |
| #define BUFF_SIZE ((NUM_LEDS + 2) * 24) // We add 24 to the end so that we have 24 (one LED) bits of 0 |
This prevents an ambiguous case where we use buff size and order precedence or formatting/syntax gets the best of us. For example:
BUFF_SIZE >>4 or similar, and we end up with (NUM_LEDS + 2) * (24 >> 4).
There was a problem hiding this comment.
Do double check that our code works properly after this change though.
My personal preference is also to use something called a constexpr although I won't force you to make that change here.
| WS2812(); | ||
| void set_led_color_by_index(uint8_t index, uint8_t green_byte, uint8_t red_byte, uint8_t blue_byte); | ||
| void set_all_led_colors(uint8_t green_byte, uint8_t red_byte, uint8_t blue_byte); | ||
| void render_leds(); |
There was a problem hiding this comment.
I would recommend you write docstrings in the .hpp file, https://github.com/UWARG/efs-zeropilot-3.5/pull/63/files Has some pretty good doxygen-style documentation. If you install a doxygen extension, it will do it for you!! (most of the way).
| set_all_led_colors(0x000808); | ||
| HAL_TIMEx_PWMN_Start_DMA(&htim1, TIM_CHANNEL_2, (uint32_t*) out_buf, BUFF_SIZE); | ||
| /* USER CODE BEGIN 3 */ | ||
| neopixel.render_leds(); |
There was a problem hiding this comment.
Does our code work if we only call render_leds() once outside of a while loop?
| } | ||
| /** Initializes the CPU, AHB and APB buses clocks | ||
| */ | ||
| RCC_ClkInitStruct.ClockType = RCC_CLOCKTYPE_HCLK | RCC_CLOCKTYPE_SYSCLK |
There was a problem hiding this comment.
Let's double check our IOC file and make sure we're stitll using the IOC at 48MHz so that we're not missing up Nick's CAN work.
| for (int i = 0; i < BUFF_SIZE; i++) { | ||
| //First and last 24 bytes LEDs get no valuable data so there's a gap between | ||
| //data transfers. | ||
| //For everything else, the buffer is filled with PWM_LO (for 0 brightness) | ||
| //by default. | ||
| if (i < 24 || i > (NUM_LEDS + 1) * 24) { | ||
| if (i < 24 || i >= (NUM_LEDS + 1) * 24) { |
|
|
||
| void WS2812::render_leds() { | ||
| HAL_TIMEx_PWMN_Start_DMA(&htim1, TIM_CHANNEL_2, (uint32_t*) out_buf, BUFF_SIZE); |
There was a problem hiding this comment.
A good addition would be adding fields to the WS2812 class init that accepts a TIM_HandleTypeDef, TIM_CHANNEL, and pointers to the buffers.
Here it doesn't really matter because the hardware is always consistent, but it would be good programming.
You can take a look at this: https://github.com/UWARG/efs-zeropilot-3.5/pull/63/files#:~:text=ICM42688(SPI_HandleTypeDef%20*%20SPI_HANDLE%2C%20GPIO_TypeDef%20*%20CS_GPIO_PORT%2C%20uint16_t%20CS_PIN)%3B
This is a class for the ws2812 Neopixel LEDs.