tedyapo / arduino-MLX90393

Arduino library for MLX90393 magnetometer sensor
MIT License
49 stars 26 forks source link

Some enhancements for the MLX library #35

Closed udoklein closed 6 years ago

udoklein commented 6 years ago

Hi tedyapo,

I did some changes to my copy of your library which you migtht want to merge back into your version:

1) For my current project I need support for the trigger mode. Thus I need to be able to set the corresponding parameters. I extended the API accordingly.

2) The cache mechanism did not really work. Every write invalidated all previouly cached values. In order to improve on this I pushed the cache down to the register level. This also decreases memory consumption in both flash and sram.

3) I found the IO with the MLX90393 somewhat hard to read as the return STATUS_ERROR visually cluttered the actual action. I tried to remedy this by always put the failure mode after the if. I find this somewhat more readable. Of course this is a matter of personal taste. If you do no like it, just throw this final commit away.

tedyapo commented 6 years ago

I like the new cache approach.

Should writeRegister() only invalidate the register written? I don't think there's any evidence of side-effect changes to other registers.

I am also wondering if readRegister() should look in the cache first, and only read from the IC if the cache is dirty.

The trigger pin and burst additions are also good.

The return error codes on the same line are fine as long as they retain the {curly braces} and don't exceed 80 characters per line.

udoklein commented 6 years ago

You could extend the caching to the registers. But I like to be more on the defensive side. My goal was not more agressive caching. I only wanted to fix the redundant reads.

The issue is: if a write fails it is unclear what exactly happens on the bus and in the chip. In general I think the lower level commands should not cache to much. Otherwise it will be hard to ensure the chip gets into a defined state once the cache is out of sync. In my opinion the overhead is acceptable as most commands will be called only once anyway.

Reasons why the cache may get out of sync: unreliable hardware.

The biggest benefit of the cache is for the floating point conversion. There I am willing to allow this risk in favour of the significantly reduced communication overhead.

As always this is a matter of personal preferences.

tedyapo commented 6 years ago

Accepted.

Nice work. Thanks!