jkelleyrtp / dw1000-rs

Rust driver crate for the Decawave DW1000 UWB transceiver
49 stars 11 forks source link

Add initial range bias implementation #58

Closed jamesmunns closed 4 years ago

jamesmunns commented 6 years ago

Initial PR for range bias correction. Usage looks a little like this:

use dwm1001::ranging::correction::range_bias::{
    correct_range_bias,
    ch_5_prf_64::CORRECTION_FACTORS,
};

let corrected_distance = correct_range_bias(
    CORRECTION_FACTORS,
    (distance_mm / 1000) as u16,
);
jamesmunns commented 6 years ago

Also, do you have any thoughts about consistent units wrt cm vs mm? I'd say I can also generate a version of the table in mm so the user doesn't have to do the conversion themselves, but that would take a few more bytes to store the correction table (on the order of 50-100 more bytes). The upside of this is saving a div operation, which can be relatively expensive.

On the other hand, the correction curve only is generally accurate to cm, so having a correction factor that is "zero extended" to mm might not make sense.

On the other other hand, we have a hardware floating point unit. Should we just be doing all of this as single precision floating point? I don't know the performance difference off the top of my head between fixed point math (incl multiplication and division), and floating point math that is HW accelerated.

EDIT: The Cortex-M4 has hardware fixed point division: https://community.arm.com/processors/b/blog/posts/divide-and-conquer - disregard most of what I said about performance. The unit consistency is probably important for UX reasons still.

hannobraun commented 6 years ago

Also, do you have any thoughts about consistent units wrt cm vs mm? I'd say I can also generate a version of the table in mm so the user doesn't have to do the conversion themselves, but that would take a few more bytes to store the correction table (on the order of 50-100 more bytes). The upside of this is saving a div operation, which can be relatively expensive.

Oh, I completely missed that. For consistency reasons I'd prefer if everything was in mm, but I'm fine with not doing this if it helps with consistency.

Ideally, we'd use something like uom, but I haven't had a chance to look into that yet.

On the other other hand, we have a hardware floating point unit. Should we just be doing all of this as single precision floating point? I don't know the performance difference off the top of my head between fixed point math (incl multiplication and division), and floating point math that is HW accelerated.

EDIT: The Cortex-M4 has hardware fixed point division: https://community.arm.com/processors/b/blog/posts/divide-and-conquer - disregard most of what I said about performance. The unit consistency is probably important for UX reasons still.

Don't forget, this is the DW1000 driver crate, not the DWM1001 BSP. We really can't make many assumption about the hardware this code is going to run on. Although everything you said is right for our own use case, of course.

jamesmunns commented 6 years ago

Good points, all around.

I think my changes will be:

jamesmunns commented 6 years ago

As a note, I'll probably finish this up once the nrf52 stuff is settled.

hannobraun commented 5 years ago

@jamesmunns I've started preparing for the first crates.io release of this crate. No idea how long it will take (still need to review the issues to decide which ones need to be fixed first), but if you want this PR to go into the initial release, now's a good time to finish it up.

jamesmunns commented 5 years ago

@hannobraun no worries if this isn't in the first release. I don't think I'll have time in the next week to complete this.

Thanks for the heads up!

hannobraun commented 5 years ago

@jamesmunns Understood. Thanks for the update!

hannobraun commented 5 years ago

I just noticed something while reviewing issues, specifically #55. The correction values that this pull request introduces are valid for the DWM1001 board and won't apply to the DW1000 in general. Other boards using the DW1000 will need to find their own calibration values. #55 has some hints about where to find more information.

At least the correction values need to live in dwm1001. I haven't thought through whether it makes sense to keep the rest of this pull request here.

Another consideration: Maybe the dwm1001 crate should apply these correction values automatically.

jamesmunns commented 4 years ago

Closing this as it is stale