updated to RGBW mode #2

mnemocron · 2020-10-27T16:48:04Z

select RGB / RGBW
added lots of documentation
updated SPL library files to newest version
updated main.c with documentation
updated main.c with gpio init()

Thank you for the great library.
I changed a few things and added a way to select between RGB and RGBW type LEDs.
This pull request is to let you know that there are improvements that you might want to copy.
I tested the code on STM8S and STM8L MCUs - but did not run your exact example project.

- select RGB / RGBW - added lots of documentation - updated SPL library files - updated main.c with documentation - updated main.c with gpio init()

j3qq4hch · 2020-10-27T16:51:29Z

Wow, this is my first PR from a stranger. Thank you for your effort, I will surely check this out.

mnemocron · 2020-10-27T17:14:48Z

No hurry. Just wanted to let you know :)
Not particularly a "good" merge request from me tbh.
Since it has so many changes - and because I moved the library files to src / inc folders which made git loose track of them.
You can also manually copy parts from my fork if you find something useful.

j3qq4hch · 2020-10-27T18:27:36Z

You know, I am thinking about removing SPL from this repo at all. Its not about SPL after all. SPL is here only because I don't like messing with registers directly (and keeping SPL here is stupid after all).
I see, you have noticed, that I had to redefine all the GPIO ports definitions somewhere inside WS2812 driver. This was done only to avoid mentioning SPL inside the lib itself (but then I needed it for some reason anyway)...
So SPL is here only for example project to work. I wonder, how can I avoid it...

mixaz · 2021-11-16T12:56:59Z

I would be good to get rid of IAR Workbench too - it is not opensource, it is not free and it is for MS Windows only ((
I see someone tried to port the library to SDCC https://github.com/jaseg/stm8_ws2811 but there's a bit of mess in example code (seemed he worked on a flame effect, that is an interesting topic by itself)

mixaz · 2021-11-16T13:15:02Z

I tried to compile @mnemocron's fork with SDCC but failed because of differences in IAR and SDCC compilers, looking at it now. @mnemocron - it is not good to pass structures by values especially in weak MCU cases. Usually it is just a waste of CPU instructions. And SDCC doesn't support it )

mnemocron · 2021-11-16T13:25:08Z

I see someone tried to port the library to SDCC

I too discovered this repo just recently. I appreciate the effort but at the current stage it is just not productive.
So I don't see myself porting this anytime soon.

it is not good to pass structures by values especially in weak MCU cases.

@mixaz are you talking about the RGBColor_t type?
Under normal circumstances I would agree that it is inefficient. But in this case it is 3 or 4 bytes, not more than a single uint32_t.
But if SDCC is incompatible with this it makes sense to change it to a pointer.

mixaz · 2021-11-16T14:02:27Z

@mnemocron, I understand that porting to SDCC is not a goal for you, that's OK. I have to work on it myself, if I need it. And I value your efforts in the PR code, actually.
I see now, RGBColor_t comes original code, not yours. I beg your pardon )

mixaz · 2021-11-18T11:09:17Z

ws2812b/inc/ws2812b_fx.h

+
+/* Includes ------------------------------------------------------------------*/
+#include "ws2812b_LLD.h"
+#include "stm8l15x.h"


stm8l15x.h is not needed

mixaz · 2021-11-18T11:11:44Z

Example project/main.c

+   so TIM4_PERIOD = (0.001 * 125000 - 1) = 124 */
+
+  /* Time base configuration */
+  TIM4_TimeBaseInit(TIM4_Prescaler_128, TIM4_PERIOD);


TIM4_PRESCALER_128, not TIM4_Prescaler_128. The same for TIM4_FLAG_Update, TIM4_IT_Update (sorry for being sourish - just spent almost a day and started losing my hair on this)

TIM4_PRESCALER_128, not TIM4_Prescaler_128. The same for TIM4_FLAG_Update, TIM4_IT_Update (sorry for being sourish - just spent almost a day and started losing my hair on this)

Do we use the same SPL? I noticed, that ST was using inconsistent naming between the STM8S and STM8L libraries. Could it be that they changed it now?

STM8_WS2812B/STM8S_StdPeriph_Driver/inc/stm8s_tim4.h

Line 57 in 6ebab04

TIM4_PRESCALER_128 = ((uint8_t)0x07)

mixaz · 2021-11-18T11:13:37Z

Example project/main.c

+  * 
+  ******************************************************************************
+  */
+/* Includes ------------------------------------------------------------------*/
 #include <string.h>
 #include <stdio.h>
 #include "stm8s_conf.h"
 #include "ws2812B_fx.h"


ws2812b_fx.h not ws2812B_fx.h - mind caps in file names. Doesn't compile on Unixes for this reason

mixaz · 2021-11-18T11:14:41Z

Example project/main.c

+static void CLK_Config(void);
+static void TIM4_Config(void);
+static void GPIO_Config(void);
+void _delay_ms(u16);


u16 is not standard. Use uint16_t instead

mixaz · 2021-11-18T11:16:00Z

Example project/main.c

- z = 0;
- GPIO_WriteReverse(GPIOB, GPIO_PIN_5);
- }
+  uptime++;


static volatile uint16_t delay_time = 0; void uptime_routine(void) { if(delay_time) delay_time--; } void _delay_ms(uint16_t wait) { delay_time = wait; while(delay_time){} }

mixaz · 2021-11-18T11:42:57Z

ws2812b/inc/ws2812b_conf.h

+
+/* ---------------------------------------------------------------------------*/
+/* GPIO Port Register Map */
+/** @todo why not use SPL definitions? */


SPL is STM library, there are other toolchains as well. It's better to not have dependencies on toolchains, and if you have them, place to separate file

mixaz · 2021-11-18T11:53:14Z

ws2812b/src/ws2812b_lld.c

+  LedsArray[LedId * COL_PER_LED +1] = Color.R;
+  LedsArray[LedId * COL_PER_LED +2] = Color.B;
+  LedsArray[LedId * COL_PER_LED +3] = Color.W;


should be under WS2812B_MODE_RGB ifdef

mixaz · 2021-11-18T11:53:51Z

ws2812b/src/ws2812b_lld.c

+// depending on RGBW or RGB type it is 3 or 4 bytes to manage
+#ifdef WS2812B_MODE_RGBW
+  #define COL_PER_LED (4)


sizeof() could be used

mixaz · 2021-11-18T11:57:32Z

ws2812b/src/ws2812b_lld.c

+#endif
+
+#ifdef WS2812B_MODE_RGB


WS2812B_MODE_RGBW and WS2812B_MODE_RGB can be disabled both, that's not OK. I would use something like #define WS2812B_MODE WS2812B_MODE_RGBW and a check for incorrect value

mixaz · 2021-11-18T12:00:33Z

ws2812b/src/ws2812b_lld.c

+  const RGBColor_t GREEN          = {  0, 255,   0,   0};
+  const RGBColor_t BLUE           = {  0,   0, 255,   0};
+  const RGBColor_t YELLOW         = {255, 255,   0,   0};
+  const RGBColor_t MAGENTA        = {255,   0, 255,   0};
+  const RGBColor_t BLACK          = {  0,   0,   0,   0};
+  const RGBColor_t WHITE          = {255, 255, 255,   0};


LLD file is about dealing with WS2812 protocol and hardware. Colors definitions should be placed to separate file then

So now you are reviewing the original source code, not the changes in the my PR.
Thanks a lot, I am learning a lot today :)
While we are at it, can you recommend any literature on "good practices in embedded software design"?
Where you learn e.g.

which code to separate into files / libraries

generally how to build robust ifdef structures

when to use/avoid certain data types

Yeah, I write comments because I spent quite a bit on this project, and I'm glad to share knowledge. I think you don't need to correct your PR, I'm going to make the changes myself. About literature - I'm an old guy, so books I read decades ago, may be not good for you )
These matters are somewhat advanced and you get it with experience. Reading others code, etc. Sorry I don't know any modern C/C++ book for embedded. I often work with Linux source code which is almost all plain C and some C++. But Linux is too complex for you (and me)), but keep your eye on RTOS (FreeRTOS, works for example on STM32, and STM supports it officially. It is part of Expressiv SDK ESP32 BTW).
You're probably working with baremetal code, next step is RTOS when you'll get out of sandbox.

I appreciate the feedback! We have briefly touched Linux and RTOS in school where the focus was on the advantages and the new special mechanisms (threads, semaphores, mutexes etc.) but never about how to organize code in a project. So I have my own style in STM32 projects but never quite know if it is good or bad practice ¯_(ツ)_/¯

Actually my own code style is far from ideal too. Ability for learning is the greatest skill I know)

I've found this STM8 port of FastLED library: https://github.com/thielj/FastLED-STM8, I'm going to try it. Seems to be using IAR compiler

mixaz · 2021-11-18T12:13:24Z

ws2812b/src/ws2812b_lld.c

+unsigned int nbLedsBytes = NB_LEDS * COL_PER_LED;
+extern unsigned char NodeId;


it's better to use sized types - uint8_t and uint16_t in this case. int and char is almost obsolete in modern C coding, they are used when size doesn't matter. Not in this case when the data is shared with ASM code

updated to RGBW mode

883f217

- select RGB / RGBW - added lots of documentation - updated SPL library files - updated main.c with documentation - updated main.c with gpio init()

update info about RGBW

6ebab04

mixaz reviewed Nov 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated to RGBW mode #2

updated to RGBW mode #2

mnemocron commented Oct 27, 2020

j3qq4hch commented Oct 27, 2020

mnemocron commented Oct 27, 2020

j3qq4hch commented Oct 27, 2020

mixaz commented Nov 16, 2021

mixaz commented Nov 16, 2021

mnemocron commented Nov 16, 2021 •

edited

Loading

mixaz commented Nov 16, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021 •

edited

Loading

mnemocron Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021 •

edited

Loading

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mnemocron Nov 18, 2021

mixaz Nov 18, 2021

mnemocron Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

mixaz Nov 18, 2021

		unsigned int nbLedsBytes = NB_LEDS * COL_PER_LED;
		extern unsigned char NodeId;

		#endif

		#ifdef WS2812B_MODE_RGB

updated to RGBW mode #2

Are you sure you want to change the base?

updated to RGBW mode #2

Conversation

mnemocron commented Oct 27, 2020

j3qq4hch commented Oct 27, 2020

mnemocron commented Oct 27, 2020

j3qq4hch commented Oct 27, 2020

mixaz commented Nov 16, 2021

mixaz commented Nov 16, 2021

mnemocron commented Nov 16, 2021 • edited Loading

mixaz commented Nov 16, 2021

Choose a reason for hiding this comment

mixaz Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixaz Nov 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnemocron commented Nov 16, 2021 •

edited

Loading

mixaz Nov 18, 2021 •

edited

Loading

mixaz Nov 18, 2021 •

edited

Loading