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

Additional accelerometers (code hygiene) #4662

Closed
dhskinner opened this issue Sep 9, 2024 Discussed in #4614 · 7 comments
Closed

Additional accelerometers (code hygiene) #4662

dhskinner opened this issue Sep 9, 2024 Discussed in #4614 · 7 comments

Comments

@dhskinner
Copy link
Contributor

dhskinner commented Sep 9, 2024

Discussed in #4614

Originally posted by dhskinner September 3, 2024
Hi - currently there's a couple of accelerometers and motion sensors supported by Meshtastic that are all in one file at .../src/AccelerometerThread.h which makes it a little awkward to add new ones

Can I suggest it could be the right time to move AccelerometerThread.h into its own folder and tease apart the code into individual classes per sensor, and to improve code hygiene - like:

/src
    |->motion
        |-> AccelerometerThread.h
        |->MPU6050.cpp
        |->MPU6050.h
        |->...etc...

The reason for raising this is I'll shortly add a PR to add the ICM-20948 (n.b. the newer big brother from the MPU-6050) but it will need a couple of new code and header files to fully enable the onboard 9-Axis digital motion processor which is quite a cool... it would be a bit odd to add a bunch of new files into the .../src/ directory

Am more than happy to do the work and submit a small PR first for some light refactoring, but.... before I start I wanted to check in - will this annoy our admins by adding new folders and moving a bit of code around or is it do-able?

[edit - as of today (Sep 3rd) I did a scan of all open PR's and there doesn't seem to be any I could spot where anyone else is making changes to AccelerometerThread.h in parallel]

@thebentern appreciate if you'd be able to take a look and let me know thoughts?

@fifieldt
Copy link
Contributor

fifieldt commented Sep 9, 2024

Have you had a look at how the temperature and other sensors are broken out at modules/telemetry/sensor? Is that the kind of approach you're thinking of following?

@thebentern
Copy link
Contributor

Looks like a good organizational strategy compared to what we have now. Excited to get more IMU and accelerometer support going

@dhskinner
Copy link
Contributor Author

Have you had a look at how the temperature and other sensors are broken out at modules/telemetry/sensor? Is that the kind of approach you're thinking of following?

Yep am thinking pretty much the same approach as for TelemetrySensor and VoltageSensor

Looks like a good organizational strategy compared to what we have now. Excited to get more IMU and accelerometer support going

OK I'll do the refactoring and submit a PR in next few days. Currently I only have one of the existing accelerometers to test against (an MPU6050) so will see how it goes and might need to ask a favour for some help to test and confirm everything works as expected when its done. Much appreciated!

@gjelsoe
Copy link
Contributor

gjelsoe commented Sep 9, 2024

I've just added a new sensor (STK8BA53) to the code for the Radiomaster Bandit but haven't made a PR yet.

Some of the code are within AccelerometerThread.h, there are also 2 new files but added as a library link in platformio.ini

@dhskinner
Copy link
Contributor Author

I've just added a new sensor (STK8BA53) to the code for the Radiomaster Bandit but haven't made a PR yet.

Ok I've put an initial refactored AccelerometerThread up here -> https://github.com/rocketflight/meshtastic_firmware/tree/ICM20948_Motion_Processor/src/motion

@gjelsoe this is all completely untested at the moment, so will take a day or three to become a PR and then a while longer to get some decent testing and everyone to be happy (I hope)... / I'll just fit in with whatever's happened in the meantime

@gjelsoe
Copy link
Contributor

gjelsoe commented Sep 9, 2024

@dhskinner I'll do my PR then.

@dhskinner
Copy link
Contributor Author

@gjelsoe Great - just took a skim through #4667 and will wait a couple of days until that's merged before submitting a new PR to refactor (and in the meantime will finish off adding ICM-20948 also) - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants