feat: rgb led functions

[CloudyPadmal]

• [x] SETRGB
• [x] SETRGB2
• [x] SETRGB3

[bessman]

Regarding the problem we discussed; you were trying to pass pointers to the RBG_LED_SetHigh, RBG_LED_SetLow functions, right? Since they're not actually functions, just #defines that look like functions, they don't have memory locations and cannot be pointed to.

I can't think of an elegant way to reduce this code duplication off the top of my head. I'll give it some more thought, but one solution would be to actually make some of the pin manipulation "functions" into actual functions. There are a few hundred of them so turning them all into real functions may be unwise due to space concerns.

[bessman]

OK, so I tried creating actual functions to set the pin states. Perhaps you already tried that? You mentioned something about the LEDs being set to solid white, which is exactly what I'm seeing.

I would guess that the reason is that the free xc16 compiler has all optimizations disabled, so it doesn't inline the functions properly. If the functions aren't inlined, that adds call overhead which messes with the timing.

[CloudyPadmal]

Hi Alex, thanks for trying it out.

Well, I also tried setting up methods to set bits LATXN = 0 and LATXN = 1 but that didn't do any difference. 🤔 and yes, the current free compiler does not have any optimisations.

[bessman]

I wonder if we could use a timer interrupt instead of the asm repeats. According to this the primary timing constraint is a maximum high time of 500 ns, i.e. 32 clock cycles in our case. That seems like it should be doable with an interrupt.

Then we could waste some time selecting the pin in a (relatively) expensive switch statement, which we could put in its own function.

[CloudyPadmal]

That sounds doable. I guess timer 3 and 4 are more vacant than 1, 2 and 5 for this..

[bessman]

I found a way to do it with macros. Here's a patch against the current commit in this PR:

diff --git a/pslab-core.X/helpers/light.c b/pslab-core.X/helpers/light.c
index adbedac..c32dd8f 100644
--- a/pslab-core.X/helpers/light.c
+++ b/pslab-core.X/helpers/light.c
@@ -4,38 +4,53 @@
 #include "../registers/system/pin_manager.h"
 #include "light.h"

-void LIGHT_RGB(uint8_t red, uint8_t green, uint8_t blue) {
-    uint8_t data[] = {green, red, blue};
-    uint8_t location;
-
-    RGB_LED_SetLow();
+#define SendZero(pin) do {\
+            pin = 1;\
+            __asm__ __volatile__("repeat #22");\
+            Nop();\
+            pin = 0;\
+            __asm__ __volatile__("repeat #51");\
+            Nop();\
+        } while(0)
+
+#define SendOne(pin) do {\
+            pin = 1;\
+            __asm__ __volatile__("repeat #45");\
+            Nop();\
+            pin = 0;\
+            __asm__ __volatile__("repeat #38");\
+            Nop();\
+        } while(0)
+
+#define SendLatch(pin) do {\
+            pin = 0;\
+            __asm__ volatile ("repeat #3264");\
+            Nop();\
+        } while(0)
+
+#define RGBCommon(red, green, blue, pin) do {\
+            uint8_t data[] = {green, red, blue};\
+            uint8_t location;\
+            \
+            SendLatch(pin);\
+            \
+            for (location = 0; location < 3; location++) {\
+                uint8_t bit;\
+                bit = data[location];\
+                uint8_t byte;\
+                for (byte = 0; byte < 8; byte++) {\
+                    if (bit & 0x80) {\
+                        SendOne(pin);\
+                    } else {\
+                        SendZero(pin);\
+                    }\
+                    bit = bit << 1;\
+                }\
+            }\
+        } while(0)

-    __asm__ volatile ("repeat #3264");
-    Nop();
-
-    for (location = 0; location < 3; location++) {
-        uint8_t bit;
-        bit = data[location];
-        uint8_t byte;
-        for (byte = 0; byte < 8; byte++) {
-            if (bit & 0x80) {
-                RGB_LED_SetHigh();
-                __asm__ __volatile__("repeat #45");
-                Nop();
-                RGB_LED_SetLow();
-                __asm__ __volatile__("repeat #38");
-                Nop();
-            } else {
-                RGB_LED_SetHigh();
-                __asm__ __volatile__("repeat #22");
-                Nop();
-                RGB_LED_SetLow();
-                __asm__ __volatile__("repeat #51");
-                Nop();
-            }
-            bit = bit << 1;
-        }
-    }
+void LIGHT_RGB(uint8_t red, uint8_t green, uint8_t blue) {
+    RGBCommon(red, green, blue, RGB_LED_Setter);
 }

 response_t LIGHT_Onboard(void) {
@@ -51,37 +66,7 @@ response_t LIGHT_Onboard(void) {
     INTERRUPT_GlobalDisable();

     for (i = 0; i < count; i = i + 3) {
-        uint8_t data[] = {colors[i+1], colors[i], colors[i+2]};
-        uint8_t location;
-
-        RGB_LED_SetLow();
-
-        __asm__ volatile ("repeat #3264");
-        Nop();
-
-        for (location = 0; location < 3; location++) {
-            uint8_t bit;
-            bit = data[location];
-            uint8_t byte;
-            for (byte = 0; byte < 8; byte++) {
-                if (bit & 0x80) {
-                    RGB_LED_SetHigh();
-                    __asm__ __volatile__("repeat #45");
-                    Nop();
-                    RGB_LED_SetLow();
-                    __asm__ __volatile__("repeat #38");
-                    Nop();
-                } else {
-                    RGB_LED_SetHigh();
-                    __asm__ __volatile__("repeat #22");
-                    Nop();
-                    RGB_LED_SetLow();
-                    __asm__ __volatile__("repeat #51");
-                    Nop();
-                }
-                bit = bit << 1;
-            }
-        }
+        RGBCommon(colors[i+1], colors[i], colors[i+2], RGB_LED_Setter);
     }

     INTERRUPT_GlobalEnable();
@@ -102,37 +87,7 @@ response_t LIGHT_One(void) {
     INTERRUPT_GlobalDisable();

     for (i = 0; i < count; i = i + 3) {
-        uint8_t data[] = {colors[i+1], colors[i], colors[i+2]};
-        uint8_t location;
-
-        SQR1_SetLow();
-
-        __asm__ volatile ("repeat #3264");
-        Nop();
-
-        for (location = 0; location < 3; location++) {
-            uint8_t bit;
-            bit = data[location];
-            uint8_t byte;
-            for (byte = 0; byte < 8; byte++) {
-                if (bit & 0x80) {
-                    SQR1_SetHigh();
-                    __asm__ __volatile__("repeat #45");
-                    Nop();
-                    SQR1_SetLow();
-                    __asm__ __volatile__("repeat #38");
-                    Nop();
-                } else {
-                    SQR1_SetHigh();
-                    __asm__ __volatile__("repeat #22");
-                    Nop();
-                    SQR1_SetLow();
-                    __asm__ __volatile__("repeat #51");
-                    Nop();
-                }
-                bit = bit << 1;
-            }
-        }
+        RGBCommon(colors[i+1], colors[i], colors[i+2], SQR1_Setter);
     }

     INTERRUPT_GlobalEnable();
@@ -153,40 +108,10 @@ response_t LIGHT_Two(void) {
     INTERRUPT_GlobalDisable();

     for (i = 0; i < count; i = i + 3) {
-        uint8_t data[] = {colors[i+1], colors[i], colors[i+2]};
-        uint8_t location;
-
-        SQR2_SetLow();
-
-        __asm__ volatile ("repeat #3264");
-        Nop();
-
-        for (location = 0; location < 3; location++) {
-            uint8_t bit;
-            bit = data[location];
-            uint8_t byte;
-            for (byte = 0; byte < 8; byte++) {
-                if (bit & 0x80) {
-                    SQR2_SetHigh();
-                    __asm__ __volatile__("repeat #45");
-                    Nop();
-                    SQR2_SetLow();
-                    __asm__ __volatile__("repeat #38");
-                    Nop();
-                } else {
-                    SQR2_SetHigh();
-                    __asm__ __volatile__("repeat #22");
-                    Nop();
-                    SQR2_SetLow();
-                    __asm__ __volatile__("repeat #51");
-                    Nop();
-                }
-                bit = bit << 1;
-            }
-        }
+        RGBCommon(colors[i+1], colors[i], colors[i+2], SQR2_Setter);
     }

     INTERRUPT_GlobalEnable();

     return SUCCESS;
-}
\ No newline at end of file
+}
diff --git a/pslab-core.X/registers/system/pin_manager.h b/pslab-core.X/registers/system/pin_manager.h
index 8144eb6..aa7c2ac 100644
--- a/pslab-core.X/registers/system/pin_manager.h
+++ b/pslab-core.X/registers/system/pin_manager.h
@@ -15,6 +15,7 @@
 #define LED_SetDigitalInput()          (_TRISB15 = 1)
 #define LED_SetDigitalOutput()         (_TRISB15 = 0)

+#define RGB_LED_Setter                 _LATB2
 #define RGB_LED_SetHigh()              (_LATB2 = 1)
 #define RGB_LED_SetLow()               (_LATB2 = 0)
 #define RGB_LED_Toggle()               (_LATB2 ^= 1)
@@ -256,6 +257,7 @@
 /*******************************************************************************
  * Wave Generator
  ******************************************************************************/
+#define SQR1_Setter                    _LATC6
 #define SQR1_SetHigh()                 (_LATC6 = 1)
 #define SQR1_SetLow()                  (_LATC6 = 0)
 #define SQR1_Toggle()                  (_LATC6 ^= 1)
@@ -263,6 +265,7 @@
 #define SQR1_SetDigitalInput()         (_TRISC6 = 1)
 #define SQR1_SetDigitalOutput()        (_TRISC6 = 0)

+#define SQR2_Setter                    _LATC7
 #define SQR2_SetHigh()                 (_LATC7 = 1)
 #define SQR2_SetLow()                  (_LATC7 = 0)
 #define SQR2_Toggle()                  (_LATC7 ^= 1)

[CloudyPadmal]

Perfect! 💯 Thanks Alex! That solves the trouble.. I've applied the patch and made a commit.

[bessman]

One more thing: This technically breaks backwards compatibility by changing the color order (from GRB to RGB) and available pins (from PGC and SQ1 to SQ1 and SQ2).

I think both of these changes make sense and we should go ahead and break backwards compatibility in this case. But as long as we're doing that, why not combine the three SETRGB commands into a single one with an added pin argument? That would also make it trivial to allow the user to select SQ3 or SQ4 as the output pin. It would also further reduce code duplication.

I wrote a suggestion for how this could be implemented in last week's meeting notes.

[bessman]

Alternatively, merge this and I'll make a PR with the aforementioned changes.

[CloudyPadmal]

Perfect. Thanks!