[DRIVER] bmp180 sensor driver implementation by aabadie · Pull Request #5078 · RIOT-OS/RIOT

aabadie · 2016-03-15T11:00:45Z

Hi,
I have a bmp180 and noticed that there's no driver implementation for it.
I tested this implementation using a Samr21-xpro (SDA is pin PA16, SCL is PA17). It works well !

kYc0o · 2016-03-15T11:14:06Z

drivers/bmp180/bmp180.c

+    printf("MB: %i\n",  (int)dev->calibration.mb);
+    printf("MC: %i\n",  (int)dev->calibration.mc);
+    printf("MD: %i\n",  (int)dev->calibration.md);
+#endif


Can you use the DEBUG() function available on debug.h?

@kYc0o : done !

aabadie · 2016-03-15T12:25:14Z

Just a side note about potential improvements: sections 3.6 and 3.7 of the bmp180 datasheet explain how to compute respectively the absolute altitude and pressure at see level.

Maybe I can add the 2 missing functions to compute those values ?

kYc0o · 2016-03-15T12:26:41Z

Is up to you! the driver can offer basic functionality but if you want to implement all the features it's always better

kYc0o · 2016-03-15T12:28:18Z

drivers/bmp180/bmp180.c

+    DEBUG("MB: %i\n",  (int)dev->calibration.mb);
+    DEBUG("MC: %i\n",  (int)dev->calibration.mc);
+    DEBUG("MD: %i\n",  (int)dev->calibration.md);
+#endif


then the #if ENABLE_DEBUG becomes unnecessary don't you think?

Indeed, I removed them.

aabadie · 2016-03-15T13:50:11Z

Is up to you! the driver can offer basic functionality but if you want to implement all the features it's always better

I added the missing functions.
Just one side question: is it possible to display float values using printffunction ? I tried %f but nothing is displayed. Maybe you have an idea.

aabadie · 2016-03-15T13:53:25Z

Maybe you have an idea.

Answering to myself: apparently using float_t type is the way to go.

jnohlgard · 2016-03-15T13:57:54Z

%f requires support in newlib, you're likely building with newlib-nano. Float printf support can be added with some linker flags though I can't remember which right now.

aabadie · 2016-03-15T14:08:07Z

%f requires support in newlib, you're likely building with newlib-nano

Indeed, but I don't know how to enable this either.

aabadie · 2016-03-15T17:09:13Z

Indeed, but I don't know how to enable this either.

It seems that I have to apply RIOT patches on newlib sources, rebuild them and re-install them, using the available scripts.
Is there any documentation somewhere ?

jnohlgard · 2016-03-15T18:08:21Z

@aabadie I don't think you need to rebuild newlib. I did this in Contiki before to get float printf support:
eistec/contiki@b5d6b25

aabadie · 2016-03-16T08:45:48Z

Thanks @gebart, I tried to add LDFLAGS += -specs=nano.specs -lc -lnosys -u _printf_float in the Makefile for the test application but still no float values displayed.

jnohlgard · 2016-03-16T08:50:48Z

@aabadie Try adding it to the end of LINKFLAGS at https://github.com/RIOT-OS/RIOT/blob/master/cpu/Makefile.include.cortexm_common#L124

aabadie · 2016-03-16T09:02:18Z

Thanks @gebart, this time it works !
I pushed an updated version of the test application.

kYc0o · 2016-03-16T14:06:04Z

Maybe we need to take care about the platforms that can support it, since not all can handle floats in this way. Let's run the CI and if everything goes ok we can merge it ;)

kYc0o · 2016-03-16T14:07:35Z

Oh! it is already complaining about the white spaces. Can you take care of this?

LudwigKnuepfer · 2016-03-16T17:33:00Z

cpu/Makefile.include.cortexm_common

 # use the nano-specs of Newlib when available
 ifeq ($(shell $(LINK) -specs=nano.specs -E - 2>/dev/null >/dev/null </dev/null ; echo $$?),0)
-export LINKFLAGS += -specs=nano.specs -lc -lnosys
+export LINKFLAGS += -specs=nano.specs -lc -lnosys -u _printf_float


I would suggest to split this out into a separate pull request.

Added #5094 for this.

LudwigKnuepfer · 2016-03-16T17:34:53Z

tests/driver_bmp180/Readme.md is empty

LudwigKnuepfer · 2016-03-16T17:35:59Z

tests/driver_bmp180/main.c

+#include "xtimer.h"
+#include "board.h"
+
+#define SLEEP   (2 * 1000 * 1000u)


maybe rename to SLEEP_2S or document..

(2S or whatever this is ;)

aabadie · 2016-03-17T10:50:36Z

@LudwigKnuepfer, I addressed most of your comments.

haukepetersen · 2016-03-18T09:30:53Z

drivers/include/bmp180.h

+ * @return                  0 on success
+ * @return                  -1 if device's I2C is not enabled in board config
+ */
+int bmp180_altitude(bmp180_t *dev, double *pressure_0, double *altitude);


I am not sure if double is a good idea here - I would strongly vote for something more ressource friendly - why not altitute in cm as int32_t? I doubt you get a better precision anyway... Same for the pressure...

Ok, altitude precision can be 1m, under I think it doesn't really make sense. I changed the returned type from double to int32_t.

haukepetersen · 2016-03-18T15:44:28Z

drivers/include/bmp180.h

+ * @return                  0 on success
+ * @return                  -1 if device's I2C is not enabled in board config
+ */
+int bmp180_altitude(bmp180_t *dev, int32_t *pressure_0, int32_t *altitude);


pressure_0 is an input, right? So why pass it as a pointer?

No particular reason (seems to be a bad practice ;) ). I changed that.

kYc0o · 2016-03-19T11:54:09Z

Murdock is complaining about nhdp tests... otherwise all other tests passed successfully. @kaspar030 is this related to the move of uhdp out of the examples?

jnohlgard · 2016-03-19T12:35:28Z

@kYc0o no, it's because of #5107

LudwigKnuepfer · 2016-03-19T18:45:06Z

tests/driver_bmp180/Readme.md

+* reads pressure (hPa) ; and prints them to the STDOUT;
+* computes the pressure at sea level (assuming the current pressure is computed
+based on Polytechnique School Campus altitude, 158m);
+* computes the altitude (given the pressure at sea level).


given the pressure at sea level => based on the pressure at sea level calculated above

given the pressure at sea level => based on the pressure at sea level calculated above

branch updated including this change.

aabadie · 2016-03-21T13:37:07Z

Just pushed a commit activating printf_float module in test application (see #5094).

haukepetersen · 2016-03-21T16:04:24Z

drivers/include/bmp180.h

+ * @brief Read pressure value from the given BMP180 device, returned in Pa
+ *
+ * @param[in]  dev          Device descriptor of BMP180 device to read from
+ * @param[out] pressure     Pressure in Pa


I think changing the output range to use 10^-1000 Pa (mPa) is probably more precise and feasible. If I am correct, the pressure is somewhere between 10 and 150 from sea-level to very high up (https://upload.wikimedia.org/wikipedia/commons/thumb/8/88/Atmospheric_Pressure_vs._Altitude.png/300px-Atmospheric_Pressure_vs._Altitude.png), so using mPa here would be more precise...

I think changing the output range to use 10^-1000 Pa (mPa) is probably more
precise and feasible. If I am correct, the pressure is somewhere between 10
and 150 from sea-level to very high up
(https://upload.wikimedia.org/wikipedia/commons/thumb/8/88/Atmospheric_Pressure_vs._Altitude.png/300px-Atmospheric_Pressure_vs._Altitude.png),
so using mPa here would be more precise...

I'm not sure to understand your comment, maybe you mean kPa ? the results here are already returned in Pa, which is 1000x more precise and corresponds to SI. Displaying the results in hPa is common in meteorology.

sorry, my bad, I was way of here... Don't know what I was thinking - never mind...

np.
Thanks to your comment I had to look again at the datasheet and this pointed me to the possibility of writing a unittest for the temperature/pressure computations: the datasheet gives ground truth values that could be used to test internal implementations of formulas in the driver.
Do you think it make sense ?

kYc0o · 2016-03-21T18:17:05Z

All the checks are successful, someone with the hardware can ACK? @LudwigKnuepfer ?

LudwigKnuepfer · 2016-03-22T17:41:45Z

it would be great if you could rewrite your commit messages to fit the module: change pattern (and still fit within the 70 character limitation for the first line of the commit message).

LudwigKnuepfer · 2016-03-22T17:42:09Z

Example: drivers/bmp180: initial import

aabadie · 2016-03-22T18:53:18Z

@LudwigKnuepfer, I edited the commit message following your suggestion.

LudwigKnuepfer · 2016-03-22T18:58:53Z

ACK

[DRIVER] bmp180 sensor driver implementation

aabadie · 2016-03-22T19:08:56Z

Merged #5078.

Thanks :)Merged #5078.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#5078 (comment)

LudwigKnuepfer · 2016-03-22T19:25:47Z

No, thank you for your contribution and endurance with all the review comments =)

kYc0o added the Area: drivers Area: Device drivers label Mar 15, 2016

kYc0o added this to the Release 2016.04 milestone Mar 15, 2016

kYc0o self-assigned this Mar 15, 2016

kYc0o reviewed Mar 15, 2016
View reviewed changes

aabadie force-pushed the bmp180_driver branch 2 times, most recently from a00fe87 to 42e657d Compare March 15, 2016 14:21

aabadie force-pushed the bmp180_driver branch from 56c70ee to a94f793 Compare March 16, 2016 09:00

kYc0o added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 16, 2016

aabadie force-pushed the bmp180_driver branch 3 times, most recently from b872573 to 69c3d3f Compare March 16, 2016 17:01

LudwigKnuepfer reviewed Mar 16, 2016
View reviewed changes

aabadie mentioned this pull request Mar 16, 2016

newlib: enabling floating point printf for cortex-m cpus #5094

Merged

aabadie force-pushed the bmp180_driver branch from 69c3d3f to 7718d74 Compare March 16, 2016 18:01

aabadie force-pushed the bmp180_driver branch 4 times, most recently from e5ca2b8 to 0536e92 Compare March 17, 2016 16:03

haukepetersen reviewed Mar 18, 2016
View reviewed changes

aabadie force-pushed the bmp180_driver branch from 0536e92 to cf4d7ab Compare March 18, 2016 15:40

haukepetersen reviewed Mar 18, 2016
View reviewed changes

LudwigKnuepfer reviewed Mar 19, 2016
View reviewed changes

kYc0o added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 20, 2016

aabadie force-pushed the bmp180_driver branch 2 times, most recently from 543c243 to 0814e2f Compare March 21, 2016 13:35

haukepetersen reviewed Mar 21, 2016
View reviewed changes

drivers/bmp180: initial implementation

941bdbb

aabadie force-pushed the bmp180_driver branch from 0814e2f to 941bdbb Compare March 22, 2016 18:51

LudwigKnuepfer added a commit that referenced this pull request Mar 22, 2016

Merge pull request #5078 from aabadie/bmp180_driver

9dcde95

[DRIVER] bmp180 sensor driver implementation

LudwigKnuepfer merged commit 9dcde95 into RIOT-OS:master Mar 22, 2016

aabadie deleted the bmp180_driver branch October 3, 2016 15:41

Conversation

aabadie commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 15, 2016

Uh oh!

kYc0o commented Mar 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 15, 2016

Uh oh!

aabadie commented Mar 15, 2016

Uh oh!

jnohlgard commented Mar 15, 2016

Uh oh!

aabadie commented Mar 15, 2016

Uh oh!

aabadie commented Mar 15, 2016

Uh oh!

jnohlgard commented Mar 15, 2016

Uh oh!

aabadie commented Mar 16, 2016

Uh oh!

jnohlgard commented Mar 16, 2016

Uh oh!

aabadie commented Mar 16, 2016

Uh oh!

kYc0o commented Mar 16, 2016

Uh oh!

kYc0o commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudwigKnuepfer commented Mar 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Mar 19, 2016

Uh oh!

jnohlgard commented Mar 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Mar 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Mar 21, 2016

Uh oh!

LudwigKnuepfer commented Mar 22, 2016

Uh oh!

LudwigKnuepfer commented Mar 22, 2016