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/os: Prototype simpler and unified async OS. #298

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
8 changes: 2 additions & 6 deletions bricks/_common/micropython.c
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@

#include <pbio/button.h>
#include <pbio/main.h>
#include <pbio/os.h>
#include <pbio/util.h>
#include <pbio/protocol.h>
#include <pbsys/main.h>
@@ -39,14 +40,9 @@
// Implementation for MICROPY_EVENT_POLL_HOOK
void pb_event_poll_hook(void) {

// Drive pbio event loop.
while (pbio_do_one_event()) {
Copy link
Member

Choose a reason for hiding this comment

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

Are we 100% sure that handling events before checking for exception wasn't important?

I couldn't find anything in git history, but I feel like it took us quite a few tries to get this function right. So I wouldn't be surprised if removing this breaks things.

}

mp_handle_pending(true);

// Platform-specific code to run on completing the poll hook.
pb_event_poll_hook_leave();
pbio_os_run_while_idle();
}

// callback for when stop button is pressed in IDE or on hub
1 change: 1 addition & 0 deletions bricks/_common/sources.mk
Original file line number Diff line number Diff line change
@@ -215,6 +215,7 @@ PBIO_SRC_C = $(addprefix lib/pbio/,\
src/motor_process.c \
src/motor/servo_settings.c \
src/observer.c \
src/os.c \
src/parent.c \
src/port.c \
src/port_dcm_pup.c \
26 changes: 5 additions & 21 deletions bricks/_common_stm32/mpconfigport.h
Original file line number Diff line number Diff line change
@@ -24,31 +24,15 @@ typedef unsigned mp_uint_t; // must be pointer size

typedef long mp_off_t;

// We have inlined IRQ functions for efficiency (they are generally
// 1 machine instruction).
//
// Note on IRQ state: you should not need to know the specific
// value of the state variable, but rather just pass the return
// value from disable_irq back to enable_irq. If you really need
// to know the machine-specific values, see irq.h.
#include <pbio/os.h>
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to avoid #includes in mpconfig* if we can.

Although if the header is very specifically crafted for such purpose (with comments that say so in the header itself), then it would be OK. Otherwise, it's easy to break things with future changes that aren't aware that mpconfig* aren't typical header files. However, we would want something just with the hook functions as os.h is way more than we should be including here.


static inline void enable_irq(mp_uint_t state) {
__set_PRIMASK(state);
}

static inline mp_uint_t disable_irq(void) {
mp_uint_t state = __get_PRIMASK();
__disable_irq();
return state;
}

#define MICROPY_BEGIN_ATOMIC_SECTION() disable_irq()
#define MICROPY_END_ATOMIC_SECTION(state) enable_irq(state)
#define MICROPY_BEGIN_ATOMIC_SECTION() pbio_os_hook_disable_irq()
#define MICROPY_END_ATOMIC_SECTION(state) pbio_os_hook_enable_irq(state)

#define MICROPY_VM_HOOK_LOOP \
do { \
extern int pbio_do_one_event(void); \
pbio_do_one_event(); \
extern bool pbio_os_run_processes_once(void); \
Copy link
Member

Choose a reason for hiding this comment

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

If we do keep the an include, then we shouldn't need thisextern declaration.

pbio_os_run_processes_once(); \
} while (0);

#define MICROPY_GC_HOOK_LOOP(i) do { \
23 changes: 12 additions & 11 deletions bricks/_common_stm32/mphalport.c
Original file line number Diff line number Diff line change
@@ -24,17 +24,18 @@ void pb_stack_get_info(char **sstack, char **estack) {
*estack = (char *)&_estack;
}

void pb_event_poll_hook_leave(void) {
// There is a possible race condition where an interrupt occurs and sets the
// Contiki poll_requested flag after all events have been processed. So we
// have a critical section where we disable interrupts and check see if there
// are any last second events. If not, we can call __WFI(), which still wakes
// up the CPU on interrupt even though interrupts are otherwise disabled.
mp_uint_t state = disable_irq();
if (!process_nevents()) {
__WFI();
}
enable_irq(state);
uint32_t pbio_os_hook_disable_irq(void) {
Copy link
Member

Choose a reason for hiding this comment

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

There is nothing micropython-specific about these hooks. I would put them in pbio/platform.

mp_uint_t flags = __get_PRIMASK();
__disable_irq();
return flags;
}

void pbio_os_hook_enable_irq(uint32_t flags) {
__set_PRIMASK(flags);
}

void pbio_os_hook_wait_for_interrupt(void) {
__WFI();
}

// using "internal" pbdrv variable
13 changes: 5 additions & 8 deletions bricks/ev3/mpconfigport.h
Original file line number Diff line number Diff line change
@@ -79,18 +79,15 @@ typedef unsigned mp_uint_t; // must be pointer size

typedef long mp_off_t;

#define CPSR_IRQ_MASK (1 << 7) // IRQ disable
#define CPSR_FIQ_MASK (1 << 6) // FIQ disable
#include <pbio/os.h>

#include <tiam1808/armv5/am1808/interrupt.h>

#define MICROPY_BEGIN_ATOMIC_SECTION() IntDisable()
#define MICROPY_END_ATOMIC_SECTION(state) IntEnable(state)
#define MICROPY_BEGIN_ATOMIC_SECTION() pbio_os_hook_disable_irq()
#define MICROPY_END_ATOMIC_SECTION(state) pbio_os_hook_enable_irq(state)

#define MICROPY_VM_HOOK_LOOP \
do { \
extern int pbio_do_one_event(void); \
pbio_do_one_event(); \
extern bool pbio_os_run_processes_once(void); \
pbio_os_run_processes_once(); \
} while (0);

#define MICROPY_GC_HOOK_LOOP(i) do { \
26 changes: 11 additions & 15 deletions bricks/ev3/mphalport.c
Original file line number Diff line number Diff line change
@@ -18,33 +18,29 @@
#include "py/mpconfig.h"
#include "py/stream.h"

#include <tiam1808/armv5/am1808/interrupt.h>

void pb_stack_get_info(char **sstack, char **estack) {
extern uint32_t _estack;
extern uint32_t _sstack;
*sstack = (char *)&_sstack;
*estack = (char *)&_estack;
}

static inline int arm_wfi(void) {
uint32_t pbio_os_hook_disable_irq(void) {
return IntDisable();
}

void pbio_os_hook_enable_irq(uint32_t flags) {
IntEnable(flags);
}

void pbio_os_hook_wait_for_interrupt(void) {
__asm volatile (
"mov r0, #0\n"
"mcr p15, 0, r0, c7, c0, 4\n" /* wait for interrupt */
::: "r0"
);
return 0;
}

void pb_event_poll_hook_leave(void) {
// There is a possible race condition where an interrupt occurs and sets the
// Contiki poll_requested flag after all events have been processed. So we
// have a critical section where we disable interrupts and check see if there
// are any last second events. If not, we can call __WFI(), which still wakes
// up the CPU on interrupt even though interrupts are otherwise disabled.
mp_uint_t state = MICROPY_BEGIN_ATOMIC_SECTION();
if (!process_nevents()) {
arm_wfi();
}
MICROPY_END_ATOMIC_SECTION(state);
}

// Core delay function that does an efficient sleep and may switch thread context.
17 changes: 11 additions & 6 deletions bricks/nxt/mpconfigport.h
Original file line number Diff line number Diff line change
@@ -79,18 +79,23 @@ typedef long mp_off_t;
// We need to provide a declaration/definition of alloca()
#include <alloca.h>

uint32_t nx_interrupts_disable(void);
void nx_interrupts_enable(uint32_t);
#include <pbio/os.h>

#define MICROPY_BEGIN_ATOMIC_SECTION() nx_interrupts_disable()
#define MICROPY_END_ATOMIC_SECTION(state) nx_interrupts_enable(state)
#define MICROPY_BEGIN_ATOMIC_SECTION() pbio_os_hook_disable_irq()
#define MICROPY_END_ATOMIC_SECTION(state) pbio_os_hook_enable_irq(state)

#define MICROPY_VM_HOOK_LOOP \
do { \
extern int pbio_do_one_event(void); \
pbio_do_one_event(); \
extern bool pbio_os_run_processes_once(void); \
pbio_os_run_processes_once(); \
} while (0);

#define MICROPY_GC_HOOK_LOOP(i) do { \
Copy link
Member

Choose a reason for hiding this comment

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

This change probably deserves a separate commit.

if (((i) & 0xf) == 0) { \
MICROPY_VM_HOOK_LOOP \
} \
} while (0)

#define MICROPY_EVENT_POLL_HOOK \
do { \
extern void pb_event_poll_hook(void); \
24 changes: 10 additions & 14 deletions bricks/nxt/mphalport.c
Original file line number Diff line number Diff line change
@@ -20,21 +20,17 @@
#include "py/mpconfig.h"
#include "py/stream.h"

void pb_event_poll_hook_leave(void) {
// There is a possible race condition where an interrupt occurs and sets
// the Contiki poll_requested flag after all events have been processed. So
// we have a critical section where we disable interrupts and check see if
// there are any last second events. If not, we can enter Idle Mode, which
// still wakes up the CPU on interrupt even though interrupts are otherwise
// disabled.
uint32_t state = nx_interrupts_disable();

if (!process_nevents()) {
// disable the processor clock which puts it in Idle Mode.
AT91C_BASE_PMC->PMC_SCDR = AT91C_PMC_PCK;
}
uint32_t pbio_os_hook_disable_irq(void) {
return nx_interrupts_disable();
}

void pbio_os_hook_enable_irq(uint32_t flags) {
nx_interrupts_enable(flags);
}

nx_interrupts_enable(state);
void pbio_os_hook_wait_for_interrupt(void) {
// disable the processor clock which puts it in Idle Mode.
AT91C_BASE_PMC->PMC_SCDR = AT91C_PMC_PCK;
}

void pb_stack_get_info(char **sstack, char **estack) {
54 changes: 19 additions & 35 deletions bricks/virtualhub/mp_port.c
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@
#include <contiki.h>

#include <pbio/main.h>
#include <pbio/os.h>
#include <pbsys/core.h>
#include <pbsys/program_stop.h>
#include <pbsys/status.h>
@@ -72,7 +73,8 @@ void pb_virtualhub_port_init(void) {
pbsys_init();

pbsys_status_set(PBIO_PYBRICKS_STATUS_USER_PROGRAM_RUNNING);
while (pbio_do_one_event()) {

while (pbio_os_run_processes_once()) {
}

pb_package_pybricks_init(true);
@@ -84,62 +86,44 @@ void pb_virtualhub_port_deinit(void) {
pb_package_pybricks_deinit();
}

// MICROPY_VM_HOOK_LOOP
void pb_virtualhub_poll(void) {
while (pbio_do_one_event()) {
}
}
// Implementation for MICROPY_EVENT_POLL_HOOK
void pb_event_poll_hook(void) {

// MICROPY_EVENT_POLL_HOOK
void pb_virtualhub_event_poll(void) {
start:
mp_handle_pending(true);

int events_handled = 0;

while (pbio_do_one_event()) {
events_handled++;
}
pbio_os_run_while_idle();
}

// If there were any pbio events handled, don't sleep because there may
// be something waiting on one of the events that was just handled.
if (events_handled) {
return;
}
// This is used instead of the uint32_t flags used in embedded builds.
static sigset_t global_origmask;

uint32_t pbio_os_hook_disable_irq(void) {
sigset_t sigmask;
sigfillset(&sigmask);
pthread_sigmask(SIG_SETMASK, &sigmask, &global_origmask);
Copy link
Member

Choose a reason for hiding this comment

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

What is stopping two threads from calling this at the same time and writing over the global state?

Better would be to return origmask.

return 0;
}

// disable "interrupts"
sigset_t origmask;
pthread_sigmask(SIG_SETMASK, &sigmask, &origmask);

if (process_nevents()) {
// something was scheduled since the event loop above
pthread_sigmask(SIG_SETMASK, &origmask, NULL);
goto start;
}
void pbio_os_hook_enable_irq(uint32_t flags) {
pthread_sigmask(SIG_SETMASK, &global_origmask, NULL);
}

void pbio_os_hook_wait_for_interrupt(void) {
struct timespec timeout = {
.tv_sec = 0,
.tv_nsec = 100000,
};

// "sleep" with "interrupts" enabled
MP_THREAD_GIL_EXIT();
pselect(0, NULL, NULL, NULL, &timeout, &origmask);
pselect(0, NULL, NULL, NULL, &timeout, &global_origmask);
MP_THREAD_GIL_ENTER();

// restore "interrupts"
pthread_sigmask(SIG_SETMASK, &origmask, NULL);
}


void pb_virtualhub_delay_us(mp_uint_t us) {
mp_uint_t start = mp_hal_ticks_us();

while (mp_hal_ticks_us() - start < us) {
pb_virtualhub_poll();
MICROPY_VM_HOOK_LOOP;
}
}

8 changes: 4 additions & 4 deletions bricks/virtualhub/mpconfigvariant.h
Original file line number Diff line number Diff line change
@@ -97,8 +97,8 @@
} while (0)

#define MICROPY_VM_HOOK_LOOP do { \
extern void pb_virtualhub_poll(void); \
pb_virtualhub_poll(); \
extern bool pbio_os_run_processes_once(void); \
pbio_os_run_processes_once(); \
} while (0);

#define MICROPY_GC_HOOK_LOOP(i) do { \
@@ -108,6 +108,6 @@
} while (0)

#define MICROPY_EVENT_POLL_HOOK do { \
extern void pb_virtualhub_event_poll(void); \
pb_virtualhub_event_poll(); \
extern void pb_event_poll_hook(void); \
pb_event_poll_hook(); \
} while (0);
Loading