Issue #225: Show UART transmission status via LEDs on the case

Here’s a strawman, where I attempt to merge your overlay concept with the benefits of offloading the concern about data synchronization.

Reservations are technically optional in the below, IFF alpha values and blending are supported. (RGB → ARGB)

For the simple ON/OFF, it’d just be:

static const requiredPixelMask = 0b0000110000; // not correct, just example
update_reserved_pixels( RGB_TAG_UART_STATUS, requiredPixelMask, 0x00FF0000); // set them to RED
Public API

typedef struct _PIXEL_BUFFER {
    CPIXEL_COLOR color[COUNT_OF_PIXELS];
} PIXEL_BUFFER;

// reserve / release pixels for update from this "owner"
bool rgb_reserve_pixels( uint32_t owner_tag, uint32_t pixel_mask );
void rgb_release_pixel_reservation( uint32_t owner_tag, uint32_t pixel_mask );
void rgb_release_all_pixel_reservations_for_owner( uint32_t owner_tag );

void update_reserved_pixels( uint32_t owner_tag, uint32_t pixel_mask, const PIXEL_BUFFER * colors );
void update_single_color( uint32_t owner_tag, uint32_t pixel_mask, CPIXEL_COLOR color );

Reservation Implementation

```c // internally to rgb.c PIXEL_BUFFER g_ForDMA; // actually used for DMA PIXEL_BUFFER g_FromSystemLevelEffects; // legacy data PIXEL_BUFFER g_ReservedPixelData; // override of output from modes, etc.

uint32_t g_ReservedPixelMask; // for performance
uint32_t g_PixelReservations[COUNT_OF_PIXELS];
LOCK g_ReservationMaskModification; // abstraction

volatile uint32_t g_UpdatePixelOutput = false;

bool rgb_reserve_pixels( uint32_t owner_tag, uint32_t pixel_mask ) {
// Need to atomically merge new mask into tracked data,
// but only if all bits in reservation request were all zero.
bool result = false;
using_lock(g_ReservationMaskModification) {
if ((pixel_mask & g_ReservedPixelMask) == 0) {
result = true;
g_ReservedPixelMask |= pixel_mask;
for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
uint32_t current_bit = 1u << i;
g_PixelReservations[i] = owner_tag;
// TODO: support alpha channel
g_ReservedPixelData[i] = 0u;
}
}
if (result) {
// flag the DMA buffer for an update in the next loop
// this ensures that, even if the DMA’d buffer took a partial update,
// the next loop will immediately restore to consistent view
g_UpdatePixelOutput = true;
}
return result;
}
void rgb_release_pixel_reservation( uint32_t owner_tag, uint32_t pixel_mask ) {
uint32_t actual_mask_to_clear = 0u;
using_lock(g_ReservationMaskModification) {
// Only clear bits that were reserved with that tag
for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
uint32_t current_bit = 1u << i;
if ((g_PixelReservations[i] == owner_tag) && (pixel_mask & current_bit)) {
assert(((1u << i) & g_ReservedPixelMask) != 0);
actual_mask_to_clear |= (1u << i);
g_PixelReservations[i] = 0u;
g_ReservedPixelData[i] = 0u;
}
}
g_ReservedPixelMask &= ~actual_mask_to_clear;
}
if (actual_mask_to_clear != 0u) {
// flag the DMA buffer for an update in the next loop
// this ensures that, even if the DMA’d buffer took a partial update,
// the next loop will immediately restore to consistent view
g_UpdatePixelOutput = true;
}
return;
}
void rgb_release_all_pixel_reservations_for_owner( uint32_t owner_tag ) {
using_lock(g_ReservationMaskModification) {
uint32_t actual_mask_to_clear = 0u;
// Remove ALL reservations for that tag
for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
uint32_t current_bit = 1u << i;
if (g_PixelReservations[i] == owner_tag) {
assert(((1u << i) & g_ReservedPixelMask) != 0);
actual_mask_to_clear |= (1u << i);
g_PixelReservations[i] = 0u;
g_ReservedPixelData[i] = 0u;
}
}
g_ReservedPixelMask &= ~actual_mask_to_clear;
}
if (actual_mask_to_clear != 0u) {
// flag the DMA buffer for an update in the next loop
// this ensures that, even if the DMA’d buffer took a partial update,
// the next loop will immediately restore to consistent view
g_UpdatePixelOutput = true;
}
return;
}

</details><hr/>


<details><summary>RGB updates API Implementation</summary><P/>

```C
void update_reserved_pixels( uint32_t owner_tag, uint32_t pixel_mask,  const PIXEL_BUFFER * colors ) {
    // GOAL: Non-blocking API
    // GOAL: DMA buffer is ALWAYS consistent:
    // 1. in a consistent state, --OR--
    // 2. flags for an update being required in the next cycle
    // NON-GOAL ... if caller is simultaneously releasing ownership and updating values ... that's their own error
    bool any_updates = false;
    for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
        uint32_t current_bit = 1u << i;
        if ((g_PixelReservations[i] == owner_tag) && (pixel_mask & current_bit)) {
            g_ReservedPixelData[i] = colors[i];
            any_updates = true;
        }
    }
    if (any_updates) {
        g_UpdatePixelOutput = true; // TODO: Verify this causes a memory barrier
    }
}
void update_single_color( uint32_t owner_tag, uint32_t pixel_mask, CPIXEL_COLOR color ) {
    // GOAL: Non-blocking API
    // GOAL: DMA buffer is ALWAYS consistent:
    // 1. in a consistent state, --OR--
    // 2. flags for an update being required in the next cycle
    // NON-GOAL ... if caller is simultaneously releasing ownership and updating values ... that's their own error
    bool any_updates = false;
    for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
        uint32_t current_bit = 1u << i;
        if ((g_PixelReservations[i] == owner_tag) && (pixel_mask & current_bit)) {
            g_ReservedPixelData[i] = colors;
            any_updates = true;
        }
    }
    if (any_updates) {
        g_UpdatePixelOutput = true; // TODO: Verify this causes a memory barrier
    }
}

RGB DMA control Implementation

// At every loop ...
if (g_UpdatePixelOutput) {
    g_UpdatePixelOutput = false; // TODO: Verify memory barrier?
    PIXEL_BUFFER tmp = {};
    memcpy(&tmp, g_FromSystemLevelEffects, sizeof(PIXEL_BUFFER));
    for (uint_fast8_t i = 0; i < COUNT_OF_PIXELS; ++i) {
        // No lock needed ... if there's an update occuring
        // from other core simultaneously, it will also flag for
        // another update ... so at most one "frame" will be
        // having inconsistent output.
        if (g_PixelReservations[i] != 0u) {
            // TODO: support alpha values for blending
            tmp.color[i] = g_ReservedPixelData[i];
        }
    }
    // Perform additional overlays
    // e.g., brightness limit
    // e.g., power usage limit
    // e.g., anything else that doesn't need locks?
    
    // Now, update the active DMA buffer
    memcpy(&g_ForDMA, &tmp, sizeof(PIXEL_BUFFER));
    // Note that this presumes the DMA of the buffer is occurring continuously...
}

1 Like

I managed to get it working kind of how I want. I’m getting close to needing the above :slight_smile: I’ve run into issues with interrupts in flight while exiting and causing freezing.

One thing I neglected was that update_pixels is also (sometimes) on an interrupt and not coop multitasking. The whole thing can probably be massively simplified to use ticks based on rgb_timer which eliminates all the interrupt issues I have.

I will start from your suggestion next.

// Note that this presumes the DMA of the buffer is occurring continuously…

    if(!dma_channel_is_busy(rgb_dma.chan)){
        //dma_channel_configure(dma_chan, &c, &pio_config.pio->txf[pio_config.sm], dma_buffer, COUNT_OF_PIXELS, true);
        dma_channel_set_read_addr(rgb_dma.chan, rgb_dma.buffer, true);
    }

The DMA buffer is not continuously running. It is expressly triggered after all the pixels values are updated.

Ah, then the earlier pseudo-code I dropped should likely change (in the “control implementation”) from:

// At every loop ...
if (g_UpdatePixelOutput) {
    g_UpdatePixelOutput = false;

To:

// At every loop ...
if (g_UpdatePixelOutput && !dma_channel_is_busy(rgb_dma.chan)) {
    g_UpdatePixelOutput = false;

Let me know if any questions arise. The above should work, but I was wrong about memory barriers (See this section for why…).

For now, a volatile global should be “good enough”, but it should be changed to a synchronization primitive similar to interrupts, with proper memory barriers / fence semantics. For now, volatile is “good enough”.

1 Like

Ok, those who update the buffer should call the following API

void PixelBuffersUpdated(void) {
    // The Data Memory Barrier (DMB) instruction ensures
    // that outstanding memory transactions complete
    // before subsequent memory transactions, but allows
    // subsequent instructions to execute w/o waiting.
    // e.g., updates to the buffers are ensured to have
    //       occurred before the global variable is set.
    __dmb(); 
    g_UpdatePixelOutput = true;
}

And the check for needing to start the DMA should call the following API

bool NeedToUpdatePixelDMA(void) {
    bool result = false;
    if (!dma_channel_is_busy(rgb_dma.chan)) {
        // IMPORTANT EDIT / BUGFIX: check value first! :)
        if (g_UpdatePixelOutput) {
            g_UpdatePixelOutput = false;
            __dmb();
            result = true;
        }
    }
    return result;
}

And if it turns out the above is still wrong, it will also make it MUCH easier to fix (vs. exposing the g_UpdatePixelOutput variable and having lots of places modify it directly).

bus_pirate5_rev10.zip

Comment by: DangerousPrototypes

Here is a test firmware with blinking LEDs triggered by RX and TX. It is not final, it crashes if there is pin movement when you exit the bridge, but it should be useful for your case.

Comment by: DangerousPrototypes