Add a way to toggle magnetometer in runtime #341
Conversation
…ucture for setting flags from the server
ImUrX
added
Type: Enhancement
There was a problem hiding this comment.
I don't like that mag status is an intrinsic property of a tracker, instead of a generic flag. I think it shouldn't be sent in SensorInfo, but should be sent separately in a more generic way like a list of sensor flags. Even if it's in SensorInfo, it should be more than just this one thing, imo.
The rest looks good.
src/network/connection.cpp
Outdated
| @@ -298,6 +298,7 @@ void Connection::sendSensorInfo(Sensor& sensor) { | |||
| MUST(sendByte(sensor.getSensorId())); | |||
| MUST(sendByte(static_cast<uint8_t>(sensor.getSensorState()))); | |||
| MUST(sendByte(static_cast<uint8_t>(sensor.getSensorType()))); | |||
| MUST(sendByte(static_cast<uint8_t>(sensor.getMagStatus()))); | |||
There was a problem hiding this comment.
I don't like that it's sent here. We're adding flags and configs, but still expanding the sensorInfo packet. It should be sent with the rest of the config.
There was a problem hiding this comment.
I mean sure, but then think of the tracker initialization in the server also, do we defer it until the sensor flag list is received? This sensor flag list when do we send it? What are we gonna use it for aside for mag?
I feel like magnetometer fits well in the sensor info as it makes sense to have it there, but if we are gonna add a flag list I want it to make sense and used for more than just magnetometer so it's something that everyone can agree on (which will take a lot longer)
There was a problem hiding this comment.
Why do we need this information during initialization?
There was a problem hiding this comment.
because it doesnt make sense for a initialized tracker to be missing data? i can add a new enum for it on server called "WAITING" and stuff 😭
aside from all that, how are we gonna make this work, do we just return a bitfield?
| @@ -27,6 +27,8 @@ | |||
| #include "sensor.h" | |||
| #include <BNO080.h> | |||
|
|
|||
| #define FLAG_SENSOR_BNO0XX_MAG_ENABLED 1 | |||
There was a problem hiding this comment.
IMO flags shouldn't be sensor-specific, I would put them all (currently only one?) in some separate enum that is common. If flag is not implemented on some sensor it shouldn't be a problem.
There was a problem hiding this comment.
where should i put the flags? In SensorConfig?
There was a problem hiding this comment.
You can pick the place yourself, no need to be blocked by people that disappeared.
There was a problem hiding this comment.
Just a quick review for now. It's been a while since I looked at the firmware, not really a fan of the amount of duplication that is present for the sensor configs. I'm wondering if we could consolidate them into types of configs rather than have a unique one for each IMU.
Co-authored-by: Lena <[email protected]>
* Untested magnetometer toggle feature for BNO08X and overal packet structure for setting flags from the server * Some build fixes * refactor(configuration): rename `CalibrationConfig` to `SensorConfig` * fix network package order * typo found * ignore clion files * finish feature * remove ota config that i used * C skill issue on defines * i have personal issues with C * do a reset before * reinit sensor * Fix remaining merge errors * remove BNO_USE_MAGNETOMETER_CORRECTION * Update src/sensors/sensor.h Co-authored-by: Lena <[email protected]> * who loves tabs * send sensorconfig instead of magdata on sensorinfo * Bump protocol and firmware version --------- Co-authored-by: Eiren Rain <[email protected]> Co-authored-by: DevMiner <[email protected]> Co-authored-by: Lena <[email protected]>
1be magnetometer in all IMUs (to simplify stuff)Fixes #230