Serial Async RX Callback only gets called once

[cptpiepmatz]

I'm trying to make uart work for me but I get weird problem with this crate.

I have written this very simple fap in C that writes a command on the serial bus and then I just count up how often that callback got called.

#include <furi.h>
#include <furi_hal.h>

#include <gui/gui.h>
#include <gui/elements.h>
#include <gui/view_dispatcher.h>
#include <gui/modules/dialog_ex.h>

#include <lib/toolbox/strint.h>

#include <notification/notification.h>
#include <notification/notification_messages.h>

#define TAG "TryUart"

uint32_t try_uart_exit(void* context) {
    UNUSED(context);
    return VIEW_NONE;
}

void try_uart_rx_callback(FuriHalSerialHandle* handle, FuriHalSerialRxEvent event, void* context) {
    FURI_LOG_I(TAG, "Got Event");
    UNUSED(event);

    uint32_t* context_int = context;
    *context_int += 1;

    if(furi_hal_serial_async_rx_available(handle)) {
        uint8_t byte = furi_hal_serial_async_rx(handle);
        FURI_LOG_I(TAG, "Got Bytes: %u", byte);
        UNUSED(byte);
    }
}

int32_t try_uart_app(void* p) {
    UNUSED(p);

    uint32_t baud_rate = 9600;
    UNUSED(baud_rate);

    Gui* gui = furi_record_open(RECORD_GUI);
    ViewDispatcher* view_dispatcher = view_dispatcher_alloc();
    View* view = view_alloc();
    view_set_previous_callback(view, try_uart_exit);
    view_dispatcher_add_view(view_dispatcher, 0, view);
    view_dispatcher_switch_to_view(view_dispatcher, 0);
    view_dispatcher_attach_to_gui(view_dispatcher, gui, ViewDispatcherTypeFullscreen);

    FURI_LOG_I(TAG, "Let's register serial");
    FuriHalSerialHandle* serial_handle = furi_hal_serial_control_acquire(FuriHalSerialIdLpuart);
    furi_assert(serial_handle);
    furi_hal_serial_init(serial_handle, 9600);
    uint32_t some_number = 42;
    furi_hal_serial_async_rx_start(serial_handle, try_uart_rx_callback, &some_number, true);

    char poll_command[] = u8"r\r\n";
    furi_hal_serial_tx(serial_handle, (const uint8_t*)poll_command, sizeof(poll_command));

    FURI_LOG_I(TAG, "And now we run");
    furi_delay_ms(2000);
    // view_dispatcher_run(view_dispatcher);
    FURI_LOG_I(TAG, "Some number: %lu", some_number);

    furi_hal_serial_async_rx_stop(serial_handle);
    furi_hal_serial_deinit(serial_handle);
    furi_hal_serial_control_release(serial_handle);

    view_dispatcher_remove_view(view_dispatcher, 0);
    view_free(view);
    view_dispatcher_free(view_dispatcher);
    furi_record_close(RECORD_GUI);

    return 0;
}

Then I implemented basically the same in Rust using this crate here.

//! Template project for Flipper Zero.
//! This app prints "Hello, Rust!" to the console then exits.

#![no_main]
#![no_std]
#![feature(ptr_as_ref_unchecked)]

// Required for panic handler
extern crate flipperzero_rt;

use core::ffi::{c_void, CStr};

use flipperzero::info;
use flipperzero_rt::{entry, manifest};

// Required for log macros
use flipperzero::__macro_support::ufmt;

use flipperzero_sys as sys;

// Define the FAP Manifest for this application
manifest!(
    name = "Try UART From Rust",
    app_version = 1,
    has_icon = false,
);

// Define the entry function
entry!(entry);

// Entry point
fn entry(_args: Option<&CStr>) -> i32 {
    unsafe { main() }
    0
}

#[no_mangle]
pub unsafe extern "C" fn exit(_: *mut c_void) -> u32 {
    0xFFFFFFFF
}


#[no_mangle]
unsafe extern "C" fn rx_callback(handle: *mut sys::FuriHalSerialHandle, _: sys::FuriHalSerialRxEvent, context: *mut c_void) {
    let context: *mut u32 = context.cast();
    let context: &mut u32 = context.as_mut_unchecked();
    *context += 1;

    if sys::furi_hal_serial_async_rx_available(handle) {
        let byte = sys::furi_hal_serial_async_rx(handle);
        info!("Got Bytes: {}", byte);
    }
}

unsafe fn main() {
    let gui: *mut sys::Gui = sys::furi_record_open(c"gui".as_ptr()).cast();
    let view_dispatcher = sys::view_dispatcher_alloc();
    let view = sys::view_alloc();
    sys::view_set_previous_callback(view, Some(exit));
    sys::view_dispatcher_add_view(view_dispatcher, 0, view);
    sys::view_dispatcher_switch_to_view(view_dispatcher, 0);
    sys::view_dispatcher_attach_to_gui(
        view_dispatcher,
        gui,
        sys::ViewDispatcherType_ViewDispatcherTypeFullscreen,
    );

    let serial_handle =
        sys::furi_hal_serial_control_acquire(sys::FuriHalSerialId_FuriHalSerialIdLpuart);
    assert!(!serial_handle.is_null());
    sys::furi_hal_serial_init(serial_handle, 9600);
    let mut some_number = 0u32;
    sys::furi_hal_serial_async_rx_start(serial_handle, Some(rx_callback), (&mut some_number as *mut u32).cast(), true);

    info!("And now we run");
    sys::furi_delay_ms(2000);
    // sys::view_dispatcher_run(view_dispatcher);
    info!("some number: {}", some_number);

    sys::furi_hal_serial_async_rx_stop(serial_handle);
    sys::furi_hal_serial_deinit(serial_handle);
    sys::furi_hal_serial_control_release(serial_handle);

    sys::view_dispatcher_remove_view(view_dispatcher, 0);
    sys::view_free(view);
    sys::view_dispatcher_free(view_dispatcher);
    sys::furi_record_close(c"gui".as_ptr());
}

For whatever reason my rx_callback in the Rust just gets called exactly once while the C variant gets called about 50 times.

[dcoles]

Hi @cptpiepmatz,

Thanks for the report and for the reproduction case. I was about to get both to build, though had to remove the info! in the Rust rx_callback as it was causing it to try to allocate memory in the RX interrupt handler.

For both the C and Rust version I see the callback being called once when the program starts and then again on any UART data sent to the device.

I did notice that the C version has:

uint32_t some_number = 42;

while the Rust version has:

let mut some_number = 0u32;

which would explain the difference in logged counter values.

[cptpiepmatz]

@dcoles thanks for your quick reply ❤️

which would explain the difference in logged counter values.

Oh, that was an accident.

though had to remove the info! in the Rust rx_callback as it was causing it to try to allocate memory in the RX interrupt handler.

I didn't know that could cause the issue. I realized that my Rust version did not include the command that would cause it to receive bytes but nevertheless, I got a crash with the included info!, without it works fine. So thank you for pointing that out.
Interestingly, the C code did not care about this and just did not log anything.

But now that leaves me with the question how could I avoid that? Is every allocation bad? Then even working with FuriString could be bad, right? And if we want to make some safe abstractions, can we check that a function doesn't do allocations? I have quite some Rust experience but working on the flipper is basically my first low level Rust experience.

[dcoles]

I didn't know that could cause the issue. I realized that my Rust version did not include the command that would cause it to receive bytes but nevertheless, I got a crash with the included info!, without it works fine. So thank you for pointing that out.

I got lucky and caught the crash message the second time I ran it. 😁

But now that leaves me with the question how could I avoid that? Is every allocation bad?

Unfortunately yes. The log! macro creates a FuriString internally which is why it explodes where the direct call to furi_log_print_format does not. Code running in an interrupt context tends to have quite tight restrictions on what can be done while interrupts are disabled. Typically the approach is to push the data somewhere and then have a worker thread process it after the interrupt finished.

The approach I took with an application using a slightly older incarnation of the Flipper UART API was to create a static buffer and static index that was written to during the interrupt. The problem with this approach is that it's fraught with risk due to the compiler optimizing away non-volatile reads or memory coherency between CPU cores. The approach taken in the upstream firmware is to use a FuriStreamBuffer and then set a flag on the reader thread.

My feeling is that having an UART abstraction that automatically creates a worker thread might be the best option for the main furi crate.

[dcoles]

I've started on wrappers for FuriStreamBuffer and furi_thread_flag_* which could be used for this, though not quite PR ready.

[cptpiepmatz]

My feeling is that having an UART abstraction that automatically creates a worker thread might be the best option for the main furi crate.

Yeah, I tried writing a trait for the callback that wraps all the pointer dereferencing and then calls the basic concept. But I don't think we can, at least with the current compiler, ensure that users (like me), don't accidentally try to allocate memory.

[cptpiepmatz]

@dcoles

I've started on wrappers for FuriStreamBuffer and furi_thread_flag_* which could be used for this, though not quite PR ready.

In my excitement I also worked on the FuriStreamBuffer implementation, see #177

[dcoles]

Nice! I was having some trouble with my code causing a BusError, so a working version would be very much appreciated. 😁

[cptpiepmatz]

After experimenting around I found out that the following seems to be absolute necessary to have the callback not crash and fulfill to not block the next occurence:

if sys::furi_hal_serial_async_rx_available(handle) {
    let _ = sys::furi_hal_serial_async_rx(handle);
}

[dcoles]

Here's my work-in-progress example of a safe serial reader using StreamBuffer including cross-thread notification.

In the rx callback you can also use the sys::FuriHalSerialRxEventData flag to see if it's safe to call furi_hal_serial_asynx_rx.

[cptpiepmatz]

That looks similar to the C code of the uart_echo debug app. However after my PR for the stream buffer lands, I think we could also just use the safe abstractions for the stream buffer to have a way simpler implementation, no?

[dcoles]

It probably does because I was making heavy reference of it. 😁

Yes, you could use StreamBuffer to handle data transfer between the interrupt handler and another thread, though you would still need to have a fair amount of unsafe code to interact with the C serial API and for the interrupt handler. What I was looking at was to provide a fully safe wrapper around the serial API. It uses a worker thread to dispatch the callback in a non-interrupt context and to handle the various error states, though I still need to think about exactly how the final API should look.