drivers: add driver for Honeywell HMC5883L magnetometer

[gschorcht]

Contribution description

This PR adds a driver for the Honeywell HMC5883L 3-axis digital magnometer as it is used for example on GY-85 9-DOF (Accelerometer ADXL345, Gyroscope ITG3200, Magnetometer HMC5883) module. Honeywell HMC5883L is a very easy to use, ultra-low-power magnetometer.

The driver implements

• basic polling mechanism
• power-down/up functions
• SAUL interface

Testing procedure

This PR includes tests/driver_hmc5883l which can be used for testing. This test application demonstrates the usage of the driver and shows the different approaches to fetch data, by using either

• the hmc5883l_read function at a lower rate than the the DOR, or
• the data-ready interrupt (DRDY).

[haukepetersen]

I just ordered some HMC5883L devices to read the gas meter in my flat. Will give this PR a review and test as soon as they arrive (hopefully during the next 2 weeks).

[gschorcht]

@haukepetersen Sounds good and it would be really great, but are we talking about the same thing? HMC5883L is a three axis compass. The use case for the use of a compass for reading gas meters is not clear to me.

[haukepetersen]

Yes, we are talking about the same thing :-) Gas meters in Germany (at least a subset of them) have their least significant digit, this analog spin wheel, encoded with a magnet at one position. So when this digit wheel spins around, you can use the HMC5883L to detect the change in magenetic flux and with that detect a revolution of the digit wheel... So at least in theory, I will let you know if it works in practice for me :-)

-> see https://www.kompf.de/tech/gascountmag.html for more info

[haukepetersen]

Just looked briefly through the code without going into too much detail yet. In general everything is straight forward, but I think there are some things we could improve on:

• the _set_reg_bit() and _get_reg_bit() functions seem very strange to me. I don't think they are needed at all. See inline comments for a proposal to remove them
• I think I would move the handling for the DRDY pin inside of the driver: initialize that GPIO pin if the configuration sets it to something different than GPIO_UNDEF. Also we could build a convenience function for read_blocking() or similar, that blocks until a sample is ready. Maybe even put all of this into a pseudo-sub-module, so its only compiled in if needed/wanted.
• using these preprocessor functions/shortcuts would not be my personal style (and its not done too often in the RIOT code base), but this is merely subjective I guess :-)

[gschorcht]

@haukepetersen Thanks for your first review.

[gschorcht]

• the _set_reg_bit() and _get_reg_bit() functions seem very strange to me. I don't think they are needed at all. See inline comments for a proposal to remove them.

Principially, I agree that is easy to OR individual bits using corresponding enum values. The same is true for reading. However, for multiple bit values, you would need to define a mask and a shift value for each register member, for example

tmp = (val << HMC5883L_REG_MODE_MD_SHIFT) & HMC5883L_REG_MODE_MD_MASK;
...
val = (tmp & HMC5883L_REG_MODE_MD_MASK) >> HMC5883L_REG_MODE_MD_SHIFT;

From my point of view, using these functions allows more readable source code, especially when many multi-bit register values ​​need to be set.

_set_bit(&tmp, HMC5883L_REG_MODE_MD, val);
...
_get_bit(val, HMC5883L_REG_MODE_MD, val);

Ok, these functions should be called _set_bit and _get_bit and they could be defined inlined or even as macros. BTW, PR #12238 will introduce such functions for common use.

• I think I would move the handling for the DRDY pin inside of the driver: initialize that GPIO pin if the configuration sets it to something different than GPIO_UNDEF. Also we could build a convenience function for read_blocking() or similar, that blocks until a sample is ready. Maybe even put all of this into a pseudo-sub-module, so its only compiled in if needed/wanted.

Good idea, I will try it in that way. There are other drivers where I did it exactly in that way.

• using these preprocessor functions/shortcuts would not be my personal style (and its not done too often in the RIOT code base), but this is merely subjective I guess :-)

Defining DEBUG_DEV and ERROR_DEV macros based on existing output macros ensures consistent output without writing down again and again exactly the same output prolog.

The same is true for EXEC_RET. But in addition, it allows to replace the four lines if (...) { DEBUG(...); return error; } statements by a single line. It's not a problem when only one register has to be written. But if dozends of registers have to be written, using the four lines if ... statements, the code would become unreadable. A good example is another driver I wrote

RIOT/drivers/vl53l1x/vl53l1x.c

Lines 610 to 655 in 39c3406

	switch (mode) {
	case VL53L1X_DIST_SHORT:
	/* from VL53L1_preset_mode_standard_ranging_short_range() */
	/* timing config */
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_A, 0x07));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_B, 0x05));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VALID_PHASE_HIGH, 0x38));
	/* dynamic config */
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD0, 0x07));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD1, 0x05));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD0, 6));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 6));
	break;
	case VL53L1X_DIST_MEDIUM:
	/* from VL53L1_preset_mode_standard_ranging() */
	/* timing config */
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_A, 0x0b));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_B, 0x09));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VALID_PHASE_HIGH, 0x78));
	/* dynamic config */
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD0, 0x0b));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD1, 0x09));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD0, 10));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 10));
	break;
	case VL53L1X_DIST_LONG:
	/* from VL53L1_preset_mode_standard_ranging_long_range() */
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_A, 0x0f));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VCSEL_PERIOD_B, 0x0d));
	EXEC_RET(_write_byte(dev, VL53L1X_RANGE_CONFIG__VALID_PHASE_HIGH, 0xb8));
	/* dynamic config */
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD0, 0x0f));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__WOI_SD1, 0x0d));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD0, 14));
	EXEC_RET(_write_byte(dev, VL53L1X_SD_CONFIG__INITIAL_PHASE_SD1, 14));
	break;
	default:
	/* unrecognized mode - do nothing */
	return VL53L1X_ERROR_INV_ARG;
	}

I know that macros should be used wisely according to the Guidelines for RIOT programming. However, in this particular case, the code size is not affected. Even more, it helps to keep the source code readable.

[haukepetersen]

Principially, I agree that is easy to OR individual bits using corresponding enum values. The same is true for reading. However, for multiple bit values, you would need to define a mask and a shift value for each register member, for example [...]

I don't see the need for shifting here. All bit positions are static, so it all boils down to OR and NOT AND operations for setting and clearing. For multi-bit values there is further no need to set the bits inidividually, they can be applied 'at-once' using their static values.

[gschorcht]

• I think I would move the handling for the DRDY pin inside of the driver: initialize that GPIO pin if the configuration sets it to something different than GPIO_UNDEF. Also we could build a convenience function for read_blocking() or similar, that blocks until a sample is ready. Maybe even put all of this into a pseudo-sub-module, so its only compiled in if needed/wanted.

After thinking about the options, I would prefer an approach as it is realized in ADS101X driver. The driver defines a function by which the interrupt is enabled. The function is called by the application and has a callback function as parameters. Defining a blocking function would require to use any synchronization mechanism like message passing or thread flags which seems to be too complex for a low level driver.

The problem is that all of our sensor drivers only realize the polling approach so that we have no examples for best practice. But using the callback mechanism for sensor interrupts should be ok.

[gschorcht]

@haukepetersen While thinking about the complexity the driver should have for interrupt handling, I am becoming unsure what the best approach is. This is probably a problem we will have with all drivers that require I2C access while handling interrupts.

The simplest approach from the driver's point of view would be for the driver to call back the application code directly from the ISR. However, this would be done in the interrupt context. That is, mutexes for synchronizing access to the I2C bus will not work and the application would have to take care of it.

Another approach would be to immediately return from the ISR and to call back the application later in the thread context. For this purpose, we introduced the "Single Interrupt Thread", which serializes interrupt events from various drivers that need to access I2C, for example. However, this approach costs an additional thread.

What do you think?

[gschorcht]

@haukepetersen Fixed most of the issues. Please take a look at how the interrupt handling is now realized. I tested the driver again and it still seems to work. The remaining open point is the question of what to do with the macros. While you do not like macros, I like them a lot because they help avoid writing down dozens of copies of the same code over and over again.

[haukepetersen]

The optional interrupt handling via callback function looks nice, just what I had mind :-)

I hope my devices arrive soon, can't wait to try it out!

[fjmolinas]

ping @haukepetersen

[benpicco]

Btw.: Is there anything that needs to be done for this chip to show up?
I also have a GY-85 9-DOF module, only connected the I2C and power lines, yet i2c_scan only reports

2020-01-14 00:03:48,417 # addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
2020-01-14 00:03:48,418 #      0 1 2 3 4 5 6 7 8 9 a b c d e f
2020-01-14 00:03:48,418 # 0x00 R R R R R R R R R R R R R R - -
2020-01-14 00:03:48,419 # 0x10 - - - - - - - - - - - - - - - -
2020-01-14 00:03:48,420 # 0x20 - - - - - - - - - - - - - - - -
2020-01-14 00:03:48,420 # 0x30 - - - - - - - - - - - - - - - -
2020-01-14 00:03:48,421 # 0x40 - - - - - - - - - - - - - - - -
2020-01-14 00:03:48,422 # 0x50 - - - X - - - - - - - - - - - -
2020-01-14 00:03:48,422 # 0x60 - - - - - - - - X - - - - - - -
2020-01-14 00:03:48,423 # 0x70 - - - - - - - - R R R R R R R R

[gschorcht]

It might be a problem of your module. I get the following output where 0x1e is the address of UMC5883L:

> i2c_scan 0
Scanning I2C device 0...
addr not ack'ed = "-", addr ack'ed = "X", addr reserved = "R", error = "E"
     0 1 2 3 4 5 6 7 8 9 a b c d e f
0x00 R R R R R R R R R R R R R R - -
0x10 - - - - - - - - - - - - - - X -
0x20 - - - - - - - - - - - - - - - -
0x30 - - - - - - - - - - - - - - - -
0x40 - - - - - - - - - - - - - - - -
0x50 - - - X - - - - - - - - - - - -
0x60 - - - - - - - - X - - - - - - -
0x70 - - - - - - - - R R R R R R R R

[gschorcht]

I have just rebased to resolve conflicts. Furthermore, I have removed the dependency of this driver from `xtimer'.

[leandrolanzieri]

@haukepetersen were your comments addressed?

[benpicco]

Looks like you've addresses all comments - please squash.

[benpicco]

The driver looks good!
Just one place where you can save a bit of ROM and a few minor style nits.
You can squash directly.

[gschorcht]

Squashed. How do we continue, it is still blocked by @haukepetersen?

[benpicco]

Please squash!

[benpicco]

The code looks good to me.

[benpicco]

Can you provide the output of tests/driver_hmc5883l, also squash.

[gschorcht]

Can you provide the output of tests/driver_hmc5883l, also squash.

Squashed. The output is:

main(): This is RIOT! (Version: 2020.04-devel-1403-ge9c0f-drivers_hmc5883l)
HMC5883L magnetometer driver test application

Initializing HMC5883L sensor
[OK]

magn [mGs] x = 152, y = 203, z = 331
head [deg] 56
magn [mGs] x = 148, y = 200, z = 332
head [deg] 56

comments have been addressed.