search

thingswebuilt / osod24_firmware

0 stars 0 forks source link

StateEstimator::estimateState() does too much in one function #39

Closed markmellors closed 6 months ago

markmellors commented 6 months ago

StateEstimator::estimateState() is massive single function, with some very long, hard to parse lines. some things that might help: [] a wrapPi function for constraining the heading to +/-pi. [] put the encoder captures into a motorspeeds/motor_position like structure, so we can iterate through all the wheels? [] split speed calc into a seperate function? [] split odometry out into a seperate function (will make it easier to do kalman filtering in future)

markmellors commented 6 months ago

re: long lines. (eg: estimatedState.driveTrainState.speeds[COMMON::MOTOR_POSITION::FRONT_LEFT] = captureFL.radians_per_second(); I think COMMON:: is redundent. MOTOR_POSITION:: might help with context. I think estimatedState is redundent. maybe it can be shortened to estimated since the property that comes afterwards is fairly obviously a state, and often includes the word state anyway.

robberwick commented 6 months ago

I think COMMON:: is redundent. MOTOR_POSITION:: might help with context. this is addressed in https://github.com/thingswebuilt/osod24_firmware/pull/38/commits/5cd6cb3e41d34513a3b82ad2ebde41698970a712 I think

markmellors commented 6 months ago

addressed by PR #40