eldruin / lsm303agr-rs

Platform agnostic Rust driver for the LSM303AGR ultra-compact high-performance eCompass module: ultra-low-power 3D accelerometer and 3D magnetometer
Apache License 2.0
18 stars 14 forks source link

Magnetometer offset cancellation #10

Closed DusterTheFirst closed 2 years ago

DusterTheFirst commented 2 years ago

Reading through the datasheet of the lsm303agr I ran upon section 4.1.2 which details how to implement magnetometer offset cancellation, but also the fact that the magnetometer can do the cancellation automatically when in continuous mode. This feature can be useful for consumers of the library so I attempted to implement it myself.

The implementation, from my observation, looks to be working (the magnetometer data is visually different when the cancellation is on versus off) but I am not very familiar with the layout of this crate and how each piece is used and works together. I tried my best to base my implementation off of the other functions found in this crate, but I cannot guarantee that it matches how the rest of the crate is designed.

DusterTheFirst commented 2 years ago

Could you add integration tests for the new methods?

Looking at other integration tests, I can understand how I would go about setting one up, but I am unsure how I would be able to check that my functions work or what to test exactly.

My thoughts would be to make sure the correct flags in the registers are set, but there is no way to check that since they are private to the struct. I could just have tests that run the functions and make sure they do not return an Err(_).

If you could clarify what specifically the tests should address that would help me a lot in making them.

eldruin commented 2 years ago

What I test is that the correct operations with the correct values are done on the I2C bus, as this should be the ultimate test. For example here. I have not tested for the status of config register variable inside the driver struct explicitly so far. It would be possible to check that this internal status behaves correctly by performing more operations in the test. Then you can see that the bits flipped by previous operations are kept and not simply overwritten.

DusterTheFirst commented 2 years ago

Is there any reason for the code duplication between src/register_address.rs and tests/common/mod.rs

Edit: Regarding my most recent changes. I implemented this:

https://github.com/eldruin/lsm303agr-rs/blob/6697faf4db13d41fb3fe2e39e137946360b3e0fd/examples/linux.rs#L1-L5

to allow the linux.rs example to compile when on non-linux platforms. This does work, but adds extra code to the example that is not important for consumers of the crate. I am unfamiliar with specifying examples in Cargo.toml, but from my limited searching it does not look like there is a way to specify examples per-platform. If there is a better way to do this (ex. exclude the example on non-linux platforms) I would gladly rather implement it otherwise.

eldruin commented 2 years ago

Is there any reason for the code duplication between src/register_address.rs and tests/common/mod.rs

The reason is that the register address is a private module that I would not want to expose and the integration tests (those on top-level under the tests folder) can only access the public interface. Doing embedded-hal-mock-based unit tests instead of integration tests is not possible because embedded-hal-mock depends on the std. So far it is just a few constants that will never change so I am fine with the duplication.

Re. running examples on non-linux platforms: what is the reason you want to do that? Is it because you are developing on a windows/mac? In this case, you can just use cross with:

cross test --target x86_64-unknown-linux-gnu

Works like a charm and the examples can be kept easy. The linux example is intended for a Raspberry Pi.

DusterTheFirst commented 2 years ago

Works like a charm and the examples can be kept easy.

Good to know. I normally develop on linux but have been using windows against my wishes. I do not have docker installed so I haven't used cross yet, but the tests should all pass in the CI.