Skip to content

Initialization hard fault memaccess #10

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

Open
christopherwun wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Initialization hard fault memaccess #10

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
44c8805
constructor init of data ptrs, defensive checks on reads
christopherwun Nov 17, 2025
3196577
remove unnec prints
christopherwun Nov 17, 2025
a8880a3
add helper to be more DRY
christopherwun Nov 17, 2025
54af197
remove data init in reset
christopherwun Nov 20, 2025
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
53 changes: 52 additions & 1 deletion shared-module/bno080/BNO080.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,24 @@ STATIC void bno080_isr_recv(void *arg) {
bno080_spi_sample(self);
}

STATIC void bno080_init_data_arrays(bno080_BNO080_obj_t *self) {
for (int i = 0; i < QUAT_DIMENSION; i++) {
self->fquat[i] = mp_obj_new_float(0.0f);
Copy link

@jocelynmass jocelynmass Nov 20, 2025

Choose a reason for hiding this comment

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

Wouldn’t this create a memory leak? I might be wrong, but from what I recall, mp_obj_new_float allocates a new object on the heap. If self->fquat[i] is not NULL, the existing pointer will be overwritten and effectively lost. The garbage collector is probably helping with this but at the performance cost.

Copy link
Collaborator Author

@christopherwun christopherwun Nov 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you are correct. I first noticed this pattern in the report function when debugging, thinking that this could be a reason for the hard fault (see fn pasted below). But, after looking into it further, since self->fquat points to an mp_obj_t array and Python (and therefore MicroPython) floats are immutable, the struct has to be updated with a new float object pointer on each update. Besides, I haven't seen any problems with memory allocation with the current implementation.

I guess an alternative solution would be to try using the native float instead of an mp_obj_t array (since we don't access self.fquat from the Python layer anyways). I think this would probably be a separate PR though - let me know your thoughts.

STATIC void bno080_report_rotation(bno080_BNO080_obj_t *self, elapsed_t timestamp, const uint8_t *pkt, int len) {
    /**
     ...
     */
    uint8_t qp = 14;  /// per section 6.5.19 Q Point = 14
    // https://en.wikipedia.org/wiki/Q_(number_format)
    float scale = pow(2.0, -qp);

    self->fquat[0] = mp_obj_new_float(READ_LE(int16_t, &pkt[4]) * scale);  // i
    self->fquat[1] = mp_obj_new_float(READ_LE(int16_t, &pkt[6]) * scale);  // j
    self->fquat[2] = mp_obj_new_float(READ_LE(int16_t, &pkt[8]) * scale);  // k
    self->fquat[3] = mp_obj_new_float(READ_LE(int16_t, &pkt[10]) * scale);  // real
}

Copy link
Collaborator Author

@christopherwun christopherwun Nov 20, 2025

Choose a reason for hiding this comment

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

I was curious whether making that change would improve performance so I tried it in this PR here as well. Haven't been able to test longer-term performance (which I assume is where we would see big changes), but the short term sampling rate is pretty much the same (and likely limited by the Python layer regardless). #11

}
for (int i = 0; i < ACCEL_DIMENSION; i++) {
self->accel[i] = mp_obj_new_float(0.0f);
}
for (int i = 0; i < GYRO_DIMENSION; i++) {
self->gyro[i] = mp_obj_new_float(0.0f);
}
for (int i = 0; i < MAG_DIMENSION; i++) {
self->mag[i] = mp_obj_new_float(0.0f);
}
for (int i = 0; i < GRAV_DIMENSION; i++) {
self->grav[i] = mp_obj_new_float(0.0f);
}
}

void common_hal_bno080_BNO080_construct(bno080_BNO080_obj_t *self, busio_spi_obj_t *bus, const mcu_pin_obj_t *cs, const mcu_pin_obj_t *rst, const mcu_pin_obj_t *ps0, const mcu_pin_obj_t *bootn, const mcu_pin_obj_t *irq) {
self->bus = bus;
self->resp = 0;
Expand All @@ -768,6 +786,9 @@ void common_hal_bno080_BNO080_construct(bno080_BNO080_obj_t *self, busio_spi_obj
common_hal_digitalio_digitalinout_construct(&self->irq, irq);
common_hal_digitalio_digitalinout_set_irq(&self->irq, EDGE_FALL, PULL_UP, bno080_isr_recv, self);

// Initialize data arrays to valid float objects (0.0)
bno080_init_data_arrays(self);

lock_bus(self);
common_hal_busio_spi_configure(self->bus, BNO_BAUDRATE, 1, 1, 8);
unlock_bus(self);
Expand Down Expand Up @@ -827,7 +848,6 @@ int common_hal_bno080_BNO080_set_feature(bno080_BNO080_obj_t *self, uint8_t feat
int rc = 0;
bno080_unary_rotation(self, feature);


const uint8_t command[17] = {
BNO080_SET_FEATURE_COMMAND,
feature,
Expand Down Expand Up @@ -868,14 +888,45 @@ mp_obj_t common_hal_bno080_BNO080_read(bno080_BNO080_obj_t *self, uint8_t report
case BNO080_SRID_GEOMAGNETIC_ROTATION_VECTOR:
case BNO080_SRID_GAME_ROTATION_VECTOR:
case BNO080_SRID_ROTATION_VECTOR:
// Defensive: check for NULLs and re-initialize if needed
for (int i = 0; i < QUAT_DIMENSION; i++) {
if (self->fquat[i] == NULL) {
mp_printf(&mp_plat_print, "Warning: fquat[%d] was NULL, reinitializing to 0.0\n", i);
self->fquat[i] = mp_obj_new_float(0.0f);
}
}
return mp_obj_new_list(QUAT_DIMENSION, self->fquat);
case BNO080_SRID_ACCELEROMETER:
for (int i = 0; i < ACCEL_DIMENSION; i++) {
if (self->accel[i] == NULL) {
mp_printf(&mp_plat_print, "Warning: accel[%d] was NULL, reinitializing to 0.0\n", i);
self->accel[i] = mp_obj_new_float(0.0f);
}
}
return mp_obj_new_list(ACCEL_DIMENSION, self->accel);
case BNO080_SRID_GYROSCOPE:
for (int i = 0; i < GYRO_DIMENSION; i++) {
if (self->gyro[i] == NULL) {
mp_printf(&mp_plat_print, "Warning: gyro[%d] was NULL, reinitializing to 0.0\n", i);
self->gyro[i] = mp_obj_new_float(0.0f);
}
}
return mp_obj_new_list(GYRO_DIMENSION, self->gyro);
case BNO080_SRID_MAGNETIC_FIELD:
for (int i = 0; i < MAG_DIMENSION; i++) {
if (self->mag[i] == NULL) {
mp_printf(&mp_plat_print, "Warning: mag[%d] was NULL, reinitializing to 0.0\n", i);
self->mag[i] = mp_obj_new_float(0.0f);
}
}
return mp_obj_new_list(MAG_DIMENSION, self->mag);
case BNO080_SRID_GRAVITY:
for (int i = 0; i < GRAV_DIMENSION; i++) {
if (self->grav[i] == NULL) {
mp_printf(&mp_plat_print, "Warning: grav[%d] was NULL, reinitializing to 0.0\n", i);
self->grav[i] = mp_obj_new_float(0.0f);
}
}
return mp_obj_new_list(GRAV_DIMENSION, self->grav);
}

Expand Down
Loading