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.

Sign up for GitHub

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 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f7e5bf1
pbio/drv/uart: Refactor async read and write.
laurensvalk Dec 1, 2024
e04f9b1
pbio/drv/ioport_debug_uart: Follow uart updates.
laurensvalk Nov 25, 2024
046af37
pbio/drv/core: Drop process autostart.
laurensvalk Nov 19, 2024
12d1bc4
pbio/drv/core: Move ADC init to normal start location.
laurensvalk Nov 19, 2024
da71e11
pbio/platform/contiki-conf: Drop autostart enable.
laurensvalk Dec 3, 2024
ffb60af
pbio/drv/ioport_debug: Drop it.
laurensvalk Dec 24, 2024
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
1 change: 0 additions & 1 deletion bricks/_common/sources.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,6 @@ PBIO_SRC_C = $(addprefix lib/pbio/,\
drv/gpio/gpio_stm32l4.c \
drv/imu/imu_lsm6ds3tr_c_stm32.c \
drv/ioport/ioport_pup.c \
drv/ioport/ioport_debug_uart.c \
drv/led/led_array_pwm.c \
drv/led/led_array.c \
drv/led/led_core.c \
Expand Down
21 changes: 21 additions & 0 deletions lib/pbio/drv/adc/adc.h
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
22 changes: 2 additions & 20 deletions lib/pbio/drv/bluetooth/bluetooth_stm32_cc2640.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,27 +43,11 @@

#include "./bluetooth_stm32_cc2640.h"

#define DEBUG_LL (0x01)
#define DEBUG_PT (0x02)

// Choose either/or DEBUG_LL | DEBUG_PT
#define DEBUG (0)
PROCESS(pbdrv_bluetooth_spi_process, "Bluetooth SPI");

#if DEBUG
#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_PT PBDRV_IOPORT_DEBUG_UART_PT_PRINTF
#else
#define DEBUG_PRINT_PT(...)
#define DEBUG_PRINT(...)
#endif
#define DEBUG_PRINT_PT(...)

// hub name goes in special section so that it can be modified when flashing firmware
__attribute__((section(".name")))
Expand Down Expand Up @@ -156,8 +140,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
2 changes: 2 additions & 0 deletions lib/pbio/drv/core.c
View file
Open in desktop
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
109 changes: 0 additions & 109 deletions lib/pbio/drv/ioport/ioport_debug_uart.c
View file
Open in desktop

This file was deleted.

57 changes: 0 additions & 57 deletions lib/pbio/drv/ioport/ioport_debug_uart.h
View file
Open in desktop

This file was deleted.

24 changes: 24 additions & 0 deletions lib/pbio/drv/legodev/legodev_pup.c
View file
Open in desktop
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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently the behavior is/was:

boot
while True:
    run device connection manager until unknown uart detected
    legodev_pup_enable_uart(dev->pins);       // <--- there was an enable but never a disable
    spawn LUMP uart thread, which runs until failure

I think the uart (buffer) is disabled initially, so in the current case the second pass was not the same as the first. Setting it back to disabled was intended to take care of your suspicion 😄

On closer look, it seems that the DCM immediately sets the buffer state again so it doesn't currently seem to make any difference.

The goal is to eventually get this into a multimodal port interface, where legodev is one of several options.

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