InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 902 forks source link

Change Acceleration Values to milli-g units rather than Raw Counts #1950

Closed jones139 closed 4 months ago

jones139 commented 5 months ago

This change adds the required scaling factors to the Bma421 accelerometer driver so that the accelerometer readings can be converted to milli-g for any of the valid instrument ranges (the default 2g up to 16g).

This addresses part of issue #1949.

To end users the change should not be significant because the scaling factor is such that for the default 2g range, the raw values are very close to the milli-g values anyway - it is much more noticeable if you switch the device range as this change will mean the returned values are unchanged (previously the acceleration values halved if you selected the +/-4g range).

I have also updated the Motion screen to display the milli-g values to check it is working.

I have checked it on a physical pinetime device using both the +/-2g and +/-4g ranges to confirm that the static value returned is close to 1000 mg.

github-actions[bot] commented 5 months ago
Build size and comparison to main: Section Size Difference
text 373172B 32B
data 940B 0B
bss 63516B 0B
FintasticMan commented 5 months ago

I like the idea of normalising the motion values so that the same value corresponds to the same force, regardless of the precision it's configured for. I do think it's a shame that conversion to milli-Gs from the raw values is a (slightly) lossy calculation. Probably not really an issue though.

I do know that the current raise to wake and lower to sleep implementations rely on 1G being 1024, and they would need to be adjusted slightly if it were 1000 instead (it would make the trig slightly harder as we use 32767 as the range for sin, which is just 1024 * 32 - 1). I presume that things such as Sleep as Android (and maybe OpenTracks) also rely on that.

Maybe we can compromise and use mibi-Gs (a term I just came up with for a 1024th of a G ;p)?

jones139 commented 5 months ago

Strangely on my device I am seeing over 1000 milli-g so I think there is an offset (or calibration error) on the accelerometer in my watch. I do not think that the 1000/1024 reduction in precision will really be significant unless you are trying to do something very precise, which is unlikely on a watch?

You are right though that we do not want to make calculations any more complex because I am already concerned about the amount of calculations we are doing, in case that is causing the issue I am seeing with not being able to achieve 25Hz sampling via the BLE MotionService.

I could make it return raw values and provide a conversion function toMillig() that would convert the values in the structure to milli-g on demand if you prefer?

Graham.

FintasticMan commented 5 months ago

I would say that normalizing the data so that 1024 == 1G makes sense, and providing a function that converts to milli-Gs. All the function would need to do is val * 1000 / 1024.

The reason you aren't seeing the 25Hz on the BLE service is likely due to the fact that the SystemTask refresh is 10Hz, and updating the service only happens every SystemTask refresh.

jones139 commented 5 months ago

Ok - I'll update it to return a 'binary milli-g' where 1G=1024 counts for all ranges - then we do not need to change anything else - it will be this evening (UK time) before I do it though.

jones139 commented 5 months ago

The reason you aren't seeing the 25Hz on the BLE service is likely due to the fact that the SystemTask refresh is 10Hz, and updating the service only happens every SystemTask refresh.

Ah, that is it then - I do see about 10Hz. I'll have a think about that because I definitely need 25Hz, but will be happy with returning several samples at each refresh, so if I can get buffering of the samples working the 10Hz refresh will be ok - thanks for pointing that out as I had missed it!

jones139 commented 5 months ago

Commit c261579 does the change to 'binary milli-g'

jones139 commented 5 months ago

The reason you aren't seeing the 25Hz on the BLE service is likely due to the fact that the SystemTask refresh is 10Hz, and updating the service only happens every SystemTask refresh.

Ah, that is it then - I do see about 10Hz. I'll have a think about that because I definitely need 25Hz, but will be happy with returning several samples at each refresh, so if I can get buffering of the samples working the 10Hz refresh will be ok - thanks for pointing that out as I had missed it!

This is more complicated than I had hoped - buffering within MotionService did not help - I still only achieved 10Hz sample frequency (but had to read the characeristic less often). I think we are polling the accelerometer from within SystemTask, rather than using the chip's FIFO and interrupt capabilities? I have put a few thoughts here: https://github.com/InfiniTimeOrg/InfiniTime/issues/1949#issuecomment-1880201731

I'd welcome any thoughts on the best way to implement higher frequency sampling without breaking other parts of InfiniTime!

FintasticMan commented 5 months ago

I've just commented in the issue. If you can move the array out of the class, into the anonymous namespace and rebase to fix the conflicts, I'd be happy to approve this PR. Then we could figure out where to go from there afterwards.

jones139 commented 5 months ago

I think this is now aligned with the latest commits to the upstream repository - is that what you meant by 'rebase'?

FintasticMan commented 5 months ago

@jones139 Could you fix the formatting issues as shown by the test-format check?

jones139 commented 5 months ago

I could not work out how to find what the issue was - do I have to run lint locally to get the list of problems?

On Wed, 10 Jan 2024, 09:11 FintasticMan, @.***> wrote:

@jones139 https://github.com/jones139 Could you fix the formatting issues as shown by the test-format check?

— Reply to this email directly, view it on GitHub https://github.com/InfiniTimeOrg/InfiniTime/pull/1950#issuecomment-1884454772, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACLSYZ6IWVIKFWN4XGV33LYNZLNHAVCNFSM6AAAAABBO4SJCWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBUGQ2TINZXGI . You are receiving this because you were mentioned.Message ID: @.***>

FintasticMan commented 5 months ago

If you go to the details page for the failing check, then click summary, there should be a file you can download called Patches. This is a zip file with patches you can apply to your code that will fix the formatting. It is probably better to install and use clang-format yourself though, as then you can format the code before you commit and push.

FintasticMan commented 4 months ago

In the end, we decided on a way of doing this in which the values aren't changed in the default configuration. 1G is still 1024. The change comes when you change the range of the accelerometer, as then 1G stays 1024. Before, 1G would be lower if the range was increased.

jones139 commented 4 months ago

I have updated the MotionService documentation to make it clear that the unit are "binary milli-g" because the units were not specified on that page previously.