<p></p>
<p><b>@benpicco</b> commented on this pull request.</p>

<p>Yea that looks better, but I'm a bit confused by the order of the bytes.</p>
<p>What is the output of <code>tests/driver_hsc</code> ?</p><hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519047694">drivers/hsc/include/hsc_internals.h</a>:</p>
<pre style='color:#555'>> +#define HSC_ADDR                    (0x28)   /* 7 bit address */
+/** @} */
+
+/**
+ * @name    HSC Max Range
+ * @{
+ */
+#define HSC_RANGE                   (40U)    /* in mbar dimension */
+/** @} */
+
+/**
+ * @name    HSC I2C Packet Readout
+ * @{
+ */
+#define HSC_PRESSURE_DATA_LENGTH    (2)      /* Pressure is stored in the first 2 bytes of data */
+#define HSC_FULL_DATA_LENGTH        (4)             /* Pressure + temperature data is 4 bytes long */
</pre>

⬇️ Suggested change
<pre style="color: #555">-#define HSC_FULL_DATA_LENGTH        (4)            /* Pressure + temperature data is 4 bytes long */
+#define HSC_FULL_DATA_LENGTH        (4)      /* Pressure + temperature data is 4 bytes long */
</pre>

<p>only use spaces for indentation</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519047852">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +    /*DEBUG("UT: %i\n", (int)*(output));*/
+
</pre>

⬇️ Suggested change
<pre style="color: #555">-    /*DEBUG("UT: %i\n", (int)*(output));*/
-
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519048700">drivers/hsc/Makefile.dep</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1 @@
+USEMODULE += xtimer
</pre>

⬇️ Suggested change
<pre style="color: #555">-USEMODULE += xtimer
+FEATURES_REQUIRED += periph_i2c
</pre>

<p>xtimer is not used by the driver</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519048982">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +
+    /* Read uncompensated pressure value */
+    _read_up(dev, &up);
+
+    /* Compute true pressure value following datasheet formulas */
+    return (int32_t)((up - 1638) * 2000 * HSC_RANGE / (14745 - 1638) - 1000 * HSC_RANGE);
+}
+
+/*------------------------------------------------------------------------------------*/
+/*                                Internal functions                                  */
+/*------------------------------------------------------------------------------------*/
+
+static int _read_ut(const hsc_t *dev, int32_t *output)
+{
+    /* Read UT (Uncompsensated Temperature value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
</pre>

⬇️ Suggested change
<pre style="color: #555">-    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+    uint8_t buf[HSC_FULL_DATA_LENGTH];
</pre>

<p>no need to set this to 0, will be overwritten anyway.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519049038">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +    return (int32_t)((up - 1638) * 2000 * HSC_RANGE / (14745 - 1638) - 1000 * HSC_RANGE);
+}
+
+/*------------------------------------------------------------------------------------*/
+/*                                Internal functions                                  */
+/*------------------------------------------------------------------------------------*/
+
+static int _read_ut(const hsc_t *dev, int32_t *output)
+{
+    /* Read UT (Uncompsensated Temperature value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+    
+    /* Acquire exclusive access */
+    i2c_acquire(DEV_I2C);
+    
+    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, &buf[0], HSC_FULL_DATA_LENGTH, 0) < 0) {
</pre>

⬇️ Suggested change
<pre style="color: #555">-    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, &buf[0], HSC_FULL_DATA_LENGTH, 0) < 0) {
+    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, buf, sizeof(buf), 0) < 0) {
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519050826">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +        return -1;
+    }
+
+    i2c_release(DEV_I2C);
+
+    *output = (((buf[2] << 8) | buf[3]) >> HSC_TEMPERATURE_SHIFT);
+        
+    /*DEBUG("UT: %i\n", (int)*(output));*/
+
+    return 0;
+}
+
+static int _read_up(const hsc_t *dev, int32_t *output)
+{
+    /* Read UP (Uncompsensated Pressure value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
</pre>

⬇️ Suggested change
<pre style="color: #555">-    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+    uint8_t buf[2];
</pre>

<p>since we are only interested in the first two bytes here</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519051188">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +
+    /* Read uncompensated temperature value */
+    _read_ut(dev, &ut);
+
+    /* Compute true temperature value following datasheet formulas */
+    return (int16_t)(ut * 200 / 2047 - 50);
+}
+
+int32_t hsc_read_pressure(const hsc_t *dev)
+{
+    int32_t up = 0;
+
+    /* Read uncompensated pressure value */
+    _read_up(dev, &up);
+
+    /* Compute true pressure value following datasheet formulas */
</pre>
<p>Which data sheet did you use?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519051995">drivers/hsc/include/hsc_internals.h</a>:</p>
<pre style='color:#555'>> + *
+ * @author      Quang Pham <phhr_quang@live.com>
+ */
+
+#ifndef HSC_INTERNALS_H
+#define HSC_INTERNALS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    HSC I2C Address
+ * @{
+ */
+#define HSC_ADDR                    (0x28)   /* 7 bit address */
</pre>

⬇️ Suggested change
<pre style="color: #555">-#define HSC_ADDR                    (0x28)   /* 7 bit address */
+#ifndef HSC_PARAM_ADDR
+#define HSC_PARAM_ADDR              (0x28)   /* 7 bit address */
+#endif
</pre>

<p>To follow the naming convention and allow to overwrite the address as sensors can have different addresses.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519052226">drivers/hsc/include/hsc_internals.h</a>:</p>
<pre style='color:#555'>> +#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @name    HSC I2C Address
+ * @{
+ */
+#define HSC_ADDR                    (0x28)   /* 7 bit address */
+/** @} */
+
+/**
+ * @name    HSC Max Range
+ * @{
+ */
+#define HSC_RANGE                   (40U)    /* in mbar dimension */
</pre>

⬇️ Suggested change
<pre style="color: #555">-#define HSC_RANGE                   (40U)    /* in mbar dimension */
+#ifndef HSC_PARAM_RANGE
+#define HSC_PARAM_RANGE             (40U)    /* in mbar dimension */
+#endif
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/15383#discussion_r519052557">drivers/hsc/hsc.c</a>:</p>
<pre style='color:#555'>> +    /* Read UP (Uncompsensated Pressure value) */
+    uint8_t buf[HSC_FULL_DATA_LENGTH] = {0};
+        
+    /* Acquire exclusive access */
+    i2c_acquire(DEV_I2C);
+    
+    if (i2c_read_bytes(DEV_I2C, DEV_ADDR, &buf[0], HSC_FULL_DATA_LENGTH, 0) < 0) {
+        i2c_release(DEV_I2C);
+        DEBUG("[hsc] read_bytes fails\n");
+
+        return -1;
+    }
+
+    i2c_release(DEV_I2C);
+
+    *output = ((buf[0] << 8) | buf[1]) & HSC_PRESSURE_MASK;
</pre>
<p>Doesn't buf[0] contain the status byte?</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/15383#pullrequestreview-525546770">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYB7SJEUUBRTPTB4HNLSOSBANANCNFSM4TKFIHDQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABE7WYBUWR7W54OSXO5ENUDSOSBANA5CNFSM4TKFIHD2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOD5JTKEQ.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/15383#pullrequestreview-525546770",
"url": "https://github.com/RIOT-OS/RIOT/pull/15383#pullrequestreview-525546770",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>