Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pbio/drv/uart: Refactor async read and write #275

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions lib/pbio/drv/adc/adc.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// SPDX-License-Identifier: MIT
// Copyright (c) 2024 The Pybricks Authors

// Internal common adc functions.

#ifndef _INTERNAL_PBDRV_ADC_H_
#define _INTERNAL_PBDRV_ADC_H_

#include <pbdrv/config.h>

#if PBDRV_CONFIG_ADC

void pbdrv_adc_init(void);

#else // PBDRV_CONFIG_ADC

#define pbdrv_adc_init()

#endif // PBDRV_CONFIG_ADC

#endif // _INTERNAL_PBDRV_ADC_H_
15 changes: 10 additions & 5 deletions lib/pbio/drv/adc/adc_stm32_hal.c
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,7 @@ static void pbdrv_adc_exit(void) {
HAL_DMA_DeInit(&pbdrv_adc_hdma);
}

PROCESS_THREAD(pbdrv_adc_process, ev, data) {
PROCESS_POLLHANDLER(pbdrv_adc_poll());
PROCESS_EXITHANDLER(pbdrv_adc_exit());

PROCESS_BEGIN();
void pbdrv_adc_init(void) {

// Timer to trigger ADC

Expand Down Expand Up @@ -149,6 +145,15 @@ PROCESS_THREAD(pbdrv_adc_process, ev, data) {
HAL_ADC_Start_DMA(&pbdrv_adc_hadc, pbdrv_adc_dma_buffer, PBIO_ARRAY_SIZE(pbdrv_adc_dma_buffer));
HAL_TIM_Base_Start(&pbdrv_adc_htim);

process_start(&pbdrv_adc_process);
}

PROCESS_THREAD(pbdrv_adc_process, ev, data) {
PROCESS_POLLHANDLER(pbdrv_adc_poll());
PROCESS_EXITHANDLER(pbdrv_adc_exit());

PROCESS_BEGIN();

while (true) {
PROCESS_WAIT_EVENT();
}
Expand Down
6 changes: 3 additions & 3 deletions lib/pbio/drv/adc/adc_stm32f0.c
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ static void pbdrv_adc_calibrate(void) {
}
}

static void pbdrv_adc_init(void) {
void pbdrv_adc_init(void) {
// enable power domain
RCC->APB2ENR |= RCC_APB2ENR_ADCEN;

Expand All @@ -61,6 +61,8 @@ static void pbdrv_adc_init(void) {
// TODO: LEGO firmware reads CH 3 during init 10 times and averages it.
// Not sure what this is measuring or what it would be used for. Perhaps
// some kind of ID resistor?

process_start(&pbdrv_adc_process);
}

// does a single conversion for the specified channel
Expand Down Expand Up @@ -91,8 +93,6 @@ PROCESS_THREAD(pbdrv_adc_process, ev, data) {

PROCESS_BEGIN();

pbdrv_adc_init();

while (true) {
PROCESS_WAIT_EVENT();
}
Expand Down
3 changes: 1 addition & 2 deletions lib/pbio/drv/bluetooth/bluetooth_btstack.c
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@
#endif

#if 0
#include <pbdrv/../../drv/ioport/ioport_debug_uart.h>
#define DEBUG_PRINT pbdrv_ioport_debug_uart_printf
#define DEBUG_PRINT(...)
#else
#define DEBUG_PRINT(...)
#endif
Expand Down
32 changes: 17 additions & 15 deletions lib/pbio/drv/bluetooth/bluetooth_stm32_cc2640.c
Original file line number Diff line number Diff line change
Expand Up @@ -43,26 +43,22 @@

#include "./bluetooth_stm32_cc2640.h"

#define DEBUG_LL (0x01)
#define DEBUG_PT (0x02)
PROCESS(pbdrv_bluetooth_spi_process, "Bluetooth SPI");

// Choose either/or DEBUG_LL | DEBUG_PT
#define DEBUG (0)
#define DEBUG_ON_LAST_UART_PORT (0)

#if DEBUG
#if DEBUG_ON_LAST_UART_PORT
#include <pbdrv/../../drv/ioport/ioport_debug_uart.h>
#endif
#if (DEBUG & DEBUG_LL)
#define DBG pbdrv_ioport_debug_uart_printf
#else
#define DBG(...)
#endif
#if (DEBUG & DEBUG_PT)
#define DEBUG_PRINT pbdrv_ioport_debug_uart_printf
#define DEBUG_PRINT(...)
#define DEBUG_PRINT_PT PBDRV_IOPORT_DEBUG_UART_PT_PRINTF
static void uart_poll_callback(pbdrv_uart_dev_t *uart) {
process_poll(&pbdrv_bluetooth_spi_process);
}
#else
#define DEBUG_PRINT_PT(...)
#define DBG(...)
#define DEBUG_PRINT(...)
#define DEBUG_PRINT_PT(...)
#endif

// hub name goes in special section so that it can be modified when flashing firmware
Expand Down Expand Up @@ -156,8 +152,6 @@ static uint16_t uart_service_handle, uart_service_end_handle, uart_rx_char_handl
// Nordic UART tx notifications enabled
static bool uart_tx_notify_en;

PROCESS(pbdrv_bluetooth_spi_process, "Bluetooth SPI");

LIST(task_queue);
static bool bluetooth_ready;
static pbdrv_bluetooth_on_event_t bluetooth_on_event;
Expand Down Expand Up @@ -2277,6 +2271,14 @@ PROCESS_THREAD(pbdrv_bluetooth_spi_process, ev, data) {

PROCESS_BEGIN();

#if DEBUG_ON_LAST_UART_PORT
// Wait for the UART to be ready for debugging.
while (pbdrv_ioport_debug_uart_init(uart_poll_callback) != PBIO_SUCCESS) {
etimer_set(&timer, 100);
PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer));
Comment on lines +2277 to +2278
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delay boot for 100 ms?

It should be fine to just yield here and skip the timer.

}
#endif // DEBUG_ON_LAST_UART_PORT

start:
// take Bluetooth chip out of reset
bluetooth_reset(RESET_STATE_OUT_HIGH);
Expand Down
2 changes: 2 additions & 0 deletions lib/pbio/drv/core.c
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <pbdrv/config.h>

#include "core.h"
#include "adc/adc.h"
#include "battery/battery.h"
#include "block_device/block_device.h"
#include "bluetooth/bluetooth.h"
Expand Down Expand Up @@ -38,6 +39,7 @@ void pbdrv_init(void) {
process_start(&etimer_process);

// the rest of the drivers should be implemented so that init order doesn't matter
pbdrv_adc_init();
pbdrv_battery_init();
pbdrv_block_device_init();
pbdrv_bluetooth_init();
Expand Down
70 changes: 10 additions & 60 deletions lib/pbio/drv/ioport/ioport_debug_uart.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,13 @@
#error "PBDRV_CONFIG_IOPORT_DEBUG_UART requires exactly one port"
#endif

static char buffer[64];
static pbdrv_uart_dev_t *debug_uart;
// Use globals for simplified debug macro inside protothreads.
struct pt debug_printf_thread;
char debug_buffer[64];
pbdrv_uart_dev_t *debug_uart;
pbio_error_t debug_err;

#define UART_TIMEOUT (250)

static pbio_error_t pbdrv_ioport_init(void) {
// UART already initialized.
if (debug_uart) {
return PBIO_SUCCESS;
}
pbio_error_t pbdrv_ioport_debug_uart_init(pbdrv_uart_poll_callback_t callback) {

// Get the debug uart from last port.
const pbdrv_ioport_pup_port_platform_data_t *port_data = &pbdrv_ioport_pup_platform_data.ports[PBDRV_CONFIG_IOPORT_NUM_DEV - 1];
Expand All @@ -39,6 +36,8 @@ static pbio_error_t pbdrv_ioport_init(void) {
return err;
}

pbdrv_uart_set_poll_callback(debug_uart, callback);

// Enable and configure uart.
const pbdrv_ioport_pup_pins_t *pins = &port_data->pins;
pbdrv_gpio_alt(&pins->uart_rx, pins->uart_alt);
Expand All @@ -50,60 +49,11 @@ static pbio_error_t pbdrv_ioport_init(void) {
return PBIO_SUCCESS;
}

pbio_error_t pbdrv_ioport_debug_uart_printf(const char *format, ...) {

// Get the debug uart.
pbio_error_t err = pbdrv_ioport_init();
if (err != PBIO_SUCCESS) {
return err;
}

// If still writing, just ignore this debug message.
err = pbdrv_uart_write_end(debug_uart);
if (err != PBIO_SUCCESS) {
return err;
}

// Buffer result.
void pbdrv_ioport_debug_uart_printf_buffer(const char *format, ...) {
va_list args;
va_start(args, format);
vsnprintf(buffer, sizeof(buffer), format, args);
vsnprintf(debug_buffer, sizeof(debug_buffer), format, args);
va_end(args);

// Write to uart.
return pbdrv_uart_write_begin(debug_uart, (uint8_t *)buffer, strlen(buffer), UART_TIMEOUT);
}

// Protothread set up here for simplified debug macro inside protothreads.
struct pt printf_thread;

PT_THREAD(pbdrv_ioport_debug_uart_printf_thread(struct pt *pt, const char *format, ...)) {
static pbio_error_t err;

PT_BEGIN(pt);

// Only print if port initialized.
err = pbdrv_ioport_init();
if (err != PBIO_SUCCESS) {
PT_EXIT(pt);
}

// Buffer result.
va_list args;
va_start(args, format);
vsnprintf(buffer, sizeof(buffer), format, args);
va_end(args);

// Wait for uart to be ready then write.
PT_WAIT_UNTIL((pt), (err = pbdrv_uart_write_begin(debug_uart, (uint8_t *)buffer, strlen(buffer), UART_TIMEOUT)) != PBIO_ERROR_AGAIN);
if (err != PBIO_SUCCESS) {
PT_EXIT(pt);
}

// Wait for uart to finish.
PT_WAIT_UNTIL((pt), (err = pbdrv_uart_write_end(debug_uart)) != PBIO_ERROR_AGAIN);

PT_END(pt);
}

#endif // PBDRV_CONFIG_IOPORT_DEBUG_UART
35 changes: 13 additions & 22 deletions lib/pbio/drv/ioport/ioport_debug_uart.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,18 @@
#include <pbio/error.h>

#include <pbdrv/config.h>
#include <pbdrv/uart.h>

#if PBDRV_CONFIG_IOPORT_DEBUG_UART

extern struct pt printf_thread;
extern char debug_buffer[];
extern struct pt debug_printf_thread;
extern pbdrv_uart_dev_t *debug_uart;
extern pbio_error_t debug_err;
Comment on lines +16 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a namespace prefix to public symbols.


/**
* Prints debug information on the last ioport.
*
* Will not print anything if the debug uart is already in use by another
* debug message, to avoid blocking.
*
* Useful outside of a protothread or when timing is more critical while
* complete information is not.
*
* @param [in] format Format string.
* @param [in] ... Arguments.
*
* @return Error code.
*/
pbio_error_t pbdrv_ioport_debug_uart_printf(const char *format, ...);
PT_THREAD(pbdrv_ioport_debug_uart_debug_printf_thread(struct pt *pt, const char *format, ...));

PT_THREAD(pbdrv_ioport_debug_uart_printf_thread(struct pt *pt, const char *format, ...));
void pbdrv_ioport_debug_uart_printf_buffer(const char *format, ...);

/**
* Spawns a task to print debug information on the last ioport.
Expand All @@ -42,13 +32,14 @@ PT_THREAD(pbdrv_ioport_debug_uart_printf_thread(struct pt *pt, const char *forma
* @param [in] ... Format string and arguments.
*/
#define PBDRV_IOPORT_DEBUG_UART_PT_PRINTF(pt, ...) \
PT_SPAWN(pt, &printf_thread, pbdrv_ioport_debug_uart_printf_thread(&printf_thread, __VA_ARGS__))
pbdrv_ioport_debug_uart_printf_buffer(__VA_ARGS__); \
if (debug_uart) { \
PT_SPAWN(pt, &debug_printf_thread, pbdrv_uart_write(&debug_printf_thread, debug_uart, (uint8_t *)debug_buffer, strlen(debug_buffer), 250, &debug_err)); \
} \

#else // PBDRV_CONFIG_IOPORT_DEBUG_UART
pbio_error_t pbdrv_ioport_debug_uart_init(pbdrv_uart_poll_callback_t callback);

static inline pbio_error_t pbdrv_ioport_debug_uart_printf(const char *format, ...) {
return PBIO_ERROR_NOT_SUPPORTED;
}
#else // PBDRV_CONFIG_IOPORT_DEBUG_UART

#define PBDRV_IOPORT_DEBUG_UART_PT_PRINTF(pt, ...)

Expand Down
24 changes: 24 additions & 0 deletions lib/pbio/drv/legodev/legodev_pup.c
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <contiki.h>

#include <pbdrv/gpio.h>
#include <pbdrv/uart.h>

#include <pbsys/status.h>

Expand Down Expand Up @@ -107,6 +108,11 @@ static void legodev_pup_enable_uart(const pbdrv_ioport_pup_pins_t *pins) {
pbdrv_gpio_out_low(&pins->uart_buf);
}

static void legodev_pup_disable_uart(const pbdrv_ioport_pup_pins_t *pins) {
// REVISIT: Move to ioport.
pbdrv_gpio_out_high(&pins->uart_buf);
}

// This is the device connection manager (dcm). It monitors the ID1 and ID2 pins
// on the port to see when devices are connected or disconnected.
// It is expected for there to be a 2ms delay between calls to this function.
Expand Down Expand Up @@ -343,6 +349,7 @@ static PT_THREAD(pbdrv_legodev_pup_thread(ext_dev_t * dev)) {
while (true) {

// Initially assume nothing is connected.
legodev_pup_disable_uart(dev->pins);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't dig into it, but this looks suspect to me. It seems like this could interfere with the device detection code that expects pins to be in a certain state.

dev->dcm.connected_type_id = PBDRV_LEGODEV_TYPE_ID_NONE;
dev->dcm.dev_id_match_count = 0;

Expand All @@ -369,6 +376,7 @@ static PT_THREAD(pbdrv_legodev_pup_thread(ext_dev_t * dev)) {
// disconnects, as observed by the UART process not getting valid data.
legodev_pup_enable_uart(dev->pins);
PT_SPAWN(&dev->pt, &dev->child, pbdrv_legodev_pup_uart_thread(&dev->child, dev->uart_dev));

}
PT_END(&dev->pt);
}
Expand Down Expand Up @@ -405,6 +413,14 @@ PROCESS_THREAD(pbio_legodev_pup_process, ev, data) {
PROCESS_END();
}

/**
* We get notified when the uart driver has completed sending or receiving data.
*/
static void uart_poll_callback(pbdrv_uart_dev_t *uart) {
// REVISIT: Only need to poll the specified uart device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a flag that gets set here so that we only poke the uart instances that need it in the poll handler.

process_poll(&pbio_legodev_pup_process);
}

void pbdrv_legodev_init(void) {
#if PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV > 0
for (uint8_t i = 0; i < PBDRV_CONFIG_LEGODEV_PUP_NUM_INT_DEV; i++) {
Expand All @@ -431,7 +447,15 @@ void pbdrv_legodev_init(void) {
pbio_dcmotor_get_dcmotor(legodev, &dcmotor);
legodev->ext_dev->uart_dev = pbdrv_legodev_pup_uart_configure(legodev_data->ioport_index, port_data->uart_driver_index, dcmotor);

// legodev driver is started after all other drivers, so we
// assume that we do not need to wait for this to be ready.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add an assert statement here so that we could at least catch this in debug builds to catch future changes that might break this assumption.

pbdrv_uart_dev_t *uart;
pbdrv_uart_get(port_data->uart_driver_index, &uart);

// Set callback for uart driver.
pbdrv_uart_set_poll_callback(uart, uart_poll_callback);
}

process_start(&pbio_legodev_pup_process);
}

Expand Down
Loading
Loading