Skip to content

Commit

Permalink
extmod/pbiodevice: keep pointer to pbio_iodev_t
Browse files Browse the repository at this point in the history 
Instead of always looking up the iodev by the port, just keep a pointer to the pbio_iodev_t.

Also add some type_id checking to ensure that the sensor is still connected before trying to perform I/O and fix possible null pointer dereference when getting type_id.

movehub fw size +36
  • Loading branch information
@dlech
dlech committed Jun 1, 2019
1 parent 4a722a1 commit c5ba549
Show file tree
Hide file tree
Showing 4 changed files with 62 additions and 57 deletions.
19 changes: 10 additions & 9 deletions extmod/modadvanced.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class IODevice():
// TODO: Use generic type for classes that just have a port property. They can also share the get_port.
typedef struct _advanced_IODevice_obj_t {
mp_obj_base_t base;
pbio_port_t port;
pbio_iodev_t *iodev;
} advanced_IODevice_obj_t;

/*
Expand All @@ -37,8 +37,9 @@ STATIC mp_obj_t advanced_IODevice_make_new(const mp_obj_type_t *type, size_t n_a
// Initialize self
mp_arg_check_num(n_args, n_kw, 1, 1, false);
advanced_IODevice_obj_t *self = m_new_obj(advanced_IODevice_obj_t);
self->base.type = (mp_obj_type_t*) type;
self->port = mp_obj_get_int(args[0]);
self->base.type = (mp_obj_type_t *)type;
pbio_port_t port = mp_obj_get_int(args[0]);
pb_assert(pbdrv_ioport_get_iodev(port, &self->iodev));
return MP_OBJ_FROM_PTR(self);
}

Expand All @@ -50,13 +51,13 @@ IODevice
STATIC void advanced_IODevice_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
advanced_IODevice_obj_t *self = MP_OBJ_TO_PTR(self_in);
mp_printf(print, qstr_str(MP_QSTR_IODevice));
mp_printf(print, " on Port.%c", self->port);
mp_printf(print, " on Port.%c", self->iodev->port);
}

STATIC mp_obj_t advanced_IODevice_type_id(const mp_obj_t self_in) {
advanced_IODevice_obj_t *self = MP_OBJ_TO_PTR(self_in);
pbio_iodev_type_id_t id;
pb_assert(pb_iodevice_get_type_id(self->port, &id));
pb_assert(pb_iodevice_get_type_id(self->iodev, &id));
return mp_obj_new_int(id);
}
STATIC MP_DEFINE_CONST_FUN_OBJ_1(advanced_IODevice_type_id_obj, advanced_IODevice_type_id);
Expand All @@ -66,12 +67,12 @@ STATIC mp_obj_t advanced_IODevice_mode(size_t n_args, const mp_obj_t *args) {
if (n_args == 1) {
// get mode
uint8_t mode;
pb_assert(pb_iodevice_get_mode(self->port, &mode));
pb_assert(pb_iodevice_get_mode(self->iodev, &mode));
return mp_obj_new_int(mode);
}
else {
// set mode
pb_assert(pb_iodevice_set_mode(self->port, mp_obj_get_int(args[1])));
pb_assert(pb_iodevice_set_mode(self->iodev, mp_obj_get_int(args[1])));
return mp_const_none;
}
}
Expand All @@ -81,11 +82,11 @@ STATIC mp_obj_t advanced_IODevice_values(size_t n_args, const mp_obj_t *args) {
advanced_IODevice_obj_t *self = MP_OBJ_TO_PTR(args[0]);
if (n_args == 1) {
// get values
return pb_iodevice_get_values(self->port);
return pb_iodevice_get_values(self->iodev);
}
else {
// set values
return pb_iodevice_set_values(self->port, args[1]);
return pb_iodevice_set_values(self->iodev, args[1]);
}
}
STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(advanced_IODevice_values_obj, 1, 2, advanced_IODevice_values);
Expand Down
43 changes: 25 additions & 18 deletions extmod/modpupdevices.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,42 @@
// Class structure for ColorAndDistSensor
typedef struct _pupdevices_ColorAndDistSensor_obj_t {
mp_obj_base_t base;
pbio_port_t port;
pbio_iodev_t *iodev;
} pupdevices_ColorAndDistSensor_obj_t;

STATIC mp_obj_t pupdevices_ColorAndDistSensor_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args ) {
// Initialize self
mp_arg_check_num(n_args, n_kw, 1, 1, false);
pupdevices_ColorAndDistSensor_obj_t *self = m_new_obj(pupdevices_ColorAndDistSensor_obj_t);
self->base.type = (mp_obj_type_t*) type;
self->port = mp_obj_get_int(args[0]);
pb_assert(pb_iodevice_set_mode(self->port, 8));
pbio_port_t port = mp_obj_get_int(args[0]);

pb_assert(pbdrv_ioport_get_iodev(port, &self->iodev));
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
pb_assert(pb_iodevice_set_mode(self->iodev, 8));
return MP_OBJ_FROM_PTR(self);
}

STATIC uint8_t pupdevices_ColorAndDistSensor_combined_mode(pbio_port_t port, uint8_t idx) {
pb_assert(pb_iodevice_set_mode(port, 8));
pbio_iodev_t *iodev;
STATIC uint8_t pupdevices_ColorAndDistSensor_combined_mode(pbio_iodev_t *iodev, uint8_t idx) {
pb_assert(pb_iodevice_set_mode(iodev, 8));
uint8_t *data;
pb_assert(pbdrv_ioport_get_iodev(port, &iodev));
pb_assert(pbio_iodev_get_raw_values(iodev, &data));
return data[idx];
}

STATIC void pupdevices_ColorAndDistSensor_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
STATIC void pupdevices_ColorAndDistSensor_print(const mp_print_t *print, mp_obj_t self_in, mp_print_kind_t kind) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
mp_printf(print, qstr_str(MP_QSTR_ColorDistanceSensor));
mp_printf(print, " on Port.%c", self->port);
mp_printf(print, " on Port.%c", self->iodev->port);
}

STATIC mp_obj_t pupdevices_ColorAndDistSensor_color(mp_obj_t self_in) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
switch(pupdevices_ColorAndDistSensor_combined_mode(self->port, 0)) {

pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);

switch(pupdevices_ColorAndDistSensor_combined_mode(self->iodev, 0)) {
case 0:
return mp_obj_new_int(PBIO_LIGHT_COLOR_BLACK);
case 3:
Expand All @@ -72,30 +77,32 @@ STATIC MP_DEFINE_CONST_FUN_OBJ_1(pupdevices_ColorAndDistSensor_color_obj, pupdev

STATIC mp_obj_t pupdevices_ColorAndDistSensor_distance(mp_obj_t self_in) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
return mp_obj_new_int(pupdevices_ColorAndDistSensor_combined_mode(self->port, 1));
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
return mp_obj_new_int(pupdevices_ColorAndDistSensor_combined_mode(self->iodev, 1));
}
MP_DEFINE_CONST_FUN_OBJ_1(pupdevices_ColorAndDistSensor_distance_obj, pupdevices_ColorAndDistSensor_distance);

STATIC mp_obj_t pupdevices_ColorAndDistSensor_reflection(mp_obj_t self_in) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
return mp_obj_new_int(pupdevices_ColorAndDistSensor_combined_mode(self->port, 3));
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
return mp_obj_new_int(pupdevices_ColorAndDistSensor_combined_mode(self->iodev, 3));
}
MP_DEFINE_CONST_FUN_OBJ_1(pupdevices_ColorAndDistSensor_reflection_obj, pupdevices_ColorAndDistSensor_reflection);

STATIC mp_obj_t pupdevices_ColorAndDistSensor_ambient(mp_obj_t self_in) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
pb_assert(pb_iodevice_set_mode(self->port, 4));
return pb_iodevice_get_values(self->port);
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
pb_assert(pb_iodevice_set_mode(self->iodev, 4));
return pb_iodevice_get_values(self->iodev);
}
MP_DEFINE_CONST_FUN_OBJ_1(pupdevices_ColorAndDistSensor_ambient_obj, pupdevices_ColorAndDistSensor_ambient);

STATIC mp_obj_t pupdevices_ColorAndDistSensor_rgb(mp_obj_t self_in) {
pupdevices_ColorAndDistSensor_obj_t *self = MP_OBJ_TO_PTR(self_in);
pb_assert(pb_iodevice_set_mode(self->port, 6));
pbio_iodev_t *iodev;
pb_iodevice_assert_type_id(self->iodev, PBIO_IODEV_TYPE_ID_COLOR_DIST_SENSOR);
pb_assert(pb_iodevice_set_mode(self->iodev, 6));
uint8_t *data;
pb_assert(pbdrv_ioport_get_iodev(self->port, &iodev));
pb_assert(pbio_iodev_get_raw_values(iodev, &data));
pb_assert(pbio_iodev_get_raw_values(self->iodev, &data));
mp_obj_t rgb[3];
for (uint8_t col = 0; col < 3; col++) {
int16_t intensity = ((*(int16_t *)(data + col * 2))*10)/44;
Expand Down
44 changes: 21 additions & 23 deletions extmod/pbiodevice.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,33 +16,35 @@
#include "pberror.h"
#include "pbiodevice.h"

pbio_error_t pb_iodevice_get_type_id(pbio_port_t port, pbio_iodev_type_id_t *id) {
pbio_iodev_t *iodev;
pbio_error_t err = pbdrv_ioport_get_iodev(port, &iodev);
if (err != PBIO_SUCCESS){
return err;
void pb_iodevice_assert_type_id(pbio_iodev_t *iodev, pbio_iodev_type_id_t type_id) {
if (!iodev->info || iodev->info->type_id != type_id) {
pb_assert(PBIO_ERROR_NO_DEV);
}
}

pbio_error_t pb_iodevice_get_type_id(pbio_iodev_t *iodev, pbio_iodev_type_id_t *id) {
if (!iodev->info) {
return PBIO_ERROR_NO_DEV;
}
*id = iodev->info->type_id;
return PBIO_SUCCESS;
}

pbio_error_t pb_iodevice_get_mode(pbio_port_t port, uint8_t *current_mode) {
pbio_iodev_t *iodev;
pbio_error_t err = pbdrv_ioport_get_iodev(port, &iodev);
if (err != PBIO_SUCCESS){
return err;
}
pbio_error_t pb_iodevice_get_mode(pbio_iodev_t *iodev, uint8_t *current_mode) {
*current_mode = iodev->mode;
return PBIO_SUCCESS;
}

pbio_error_t pb_iodevice_set_mode(pbio_port_t port, uint8_t new_mode) {
pbio_iodev_t *iodev;
pbio_error_t err = pbdrv_ioport_get_iodev(port, &iodev);
// Return error if any. Return success if mode already set.
if (err != PBIO_SUCCESS || iodev->mode == new_mode){
return err;
pbio_error_t pb_iodevice_set_mode(pbio_iodev_t *iodev, uint8_t new_mode) {
pbio_error_t err;

// FIXME: it would be better to do this check on a per-sensor basis since
// some sensors use setting the mode as a oneshot to update the sensor
// value - e.g. LEGO EV3 Ultrasonic sensor in certain modes.
if (iodev->mode == new_mode){
return PBIO_SUCCESS;
}

err = pbio_iodev_set_mode(iodev, new_mode);
// Wait for mode change to complete unless there was an error.
while (err == PBIO_SUCCESS && iodev->mode != new_mode) {
Expand All @@ -51,14 +53,12 @@ pbio_error_t pb_iodevice_set_mode(pbio_port_t port, uint8_t new_mode) {
return err;
}

mp_obj_t pb_iodevice_get_values(pbio_port_t port) {
mp_obj_t pb_iodevice_get_values(pbio_iodev_t *iodev) {
mp_obj_t values[PBIO_IODEV_MAX_DATA_SIZE];
pbio_iodev_t *iodev;
uint8_t *data;
uint8_t len, i;
pbio_iodev_data_type_t type;

pb_assert(pbdrv_ioport_get_iodev(port, &iodev));
pb_assert(pbio_iodev_get_raw_values(iodev, &data));
pb_assert(pbio_iodev_get_bin_format(iodev, &len, &type));

Expand Down Expand Up @@ -100,14 +100,12 @@ mp_obj_t pb_iodevice_get_values(pbio_port_t port) {
return values[0];
}

mp_obj_t pb_iodevice_set_values(pbio_port_t port, mp_obj_t values) {
mp_obj_t pb_iodevice_set_values(pbio_iodev_t *iodev, mp_obj_t values) {
uint8_t data[PBIO_IODEV_MAX_DATA_SIZE];
pbio_iodev_t *iodev;
mp_obj_t *items;
uint8_t len, i;
pbio_iodev_data_type_t type;

pb_assert(pbdrv_ioport_get_iodev(port, &iodev));
pb_assert(pbio_iodev_get_bin_format(iodev, &len, &type));

// if we only have one value, it doesn't have to be a tuple/list
Expand Down
13 changes: 6 additions & 7 deletions extmod/pbiodevice.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,14 @@

#include <stdint.h>

#include <pbdrv/ioport.h>
#include <pbio/error.h>
#include <pbio/iodev.h>
#include <pbio/port.h>

#include "py/obj.h"

pbio_error_t pb_iodevice_get_type_id(pbio_port_t port, pbio_iodev_type_id_t *id);
pbio_error_t pb_iodevice_get_mode(pbio_port_t port, uint8_t *current_mode);
pbio_error_t pb_iodevice_set_mode(pbio_port_t port, uint8_t new_mode);
mp_obj_t pb_iodevice_get_values(pbio_port_t port);
mp_obj_t pb_iodevice_set_values(pbio_port_t port, mp_obj_t values);
void pb_iodevice_assert_type_id(pbio_iodev_t *iodev, pbio_iodev_type_id_t type_id);
pbio_error_t pb_iodevice_get_type_id(pbio_iodev_t *iodev, pbio_iodev_type_id_t *id);
pbio_error_t pb_iodevice_get_mode(pbio_iodev_t *iodev, uint8_t *current_mode);
pbio_error_t pb_iodevice_set_mode(pbio_iodev_t *iodev, uint8_t new_mode);
mp_obj_t pb_iodevice_get_values(pbio_iodev_t *iodev);
mp_obj_t pb_iodevice_set_values(pbio_iodev_t *iodev, mp_obj_t values);

0 comments on commit c5ba549

Please sign in to comment.