drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor #3755

daniel-k · 2015-09-01T15:21:10Z

This PR add a device driver for the AT30TSE75x series temperature sensors included in the ATIO1-XPRO dev board. There's also a EEPROM on the same chips, for which I will add driver support later.

This PR weakly depends on #3751, but should work without error feedback nevertheless.

PeterKietzmann · 2015-09-01T20:59:19Z

@daniel-k thanks for the nice driver! Is the review urgent? Generally I would review this PR, but currently this may take some time. Does someone own this piece of hardware? Maybe @kaspar030?

PeterKietzmann · 2015-09-01T21:01:15Z

Ah, and I will assign myself for now. If someone else has capacities for a review, feel free to assign yourself :-) !

daniel-k · 2015-09-01T23:47:15Z

No, it's not urgent. Nothing depends on it, so take your time. Maybe the EEPROM driver will be finished before this get's merged (depending on my motivation) but I'd open a separate PR anyway.

daniel-k · 2015-09-02T13:25:44Z

@PeterKietzmann

I'm not sure if it's a good idea to call i2c_init_master() inside the driver. Shouldn't this be called by the user code, because every i2c driver would then initialize the master device again? I'm asking because other driver are doing the same.

PeterKietzmann · 2015-09-02T15:47:25Z

I know I know.. Please check #2528?

PeterKietzmann · 2015-09-29T15:39:04Z

drivers/at30tse75x/at30tse75x.c

+
+
+#include "periph/i2c.h"
+#include "hwtimer.h"


@daniel-k, sorry for the delay. Could you adapt the implementation to use xtimer please

Done, see ~~0d171a8~~ 3b9b21c.

PeterKietzmann · 2015-09-29T16:02:25Z

drivers/at30tse75x/at30tse75x.c

+    }
+
+    switch(mode) {
+    case AT30TSE75X_MODE_ONE_SHOT:


I'm not sure out of my head if it's written in our coding conventions, but I think that case is indented against switch mostly

I'm just aware of these coding conventions and they don't mention this. But I don't really have an opinion on this, so I can adapt it if you wish.

EDIT: Linux kernel coding conventions even say to not indent case :P

Honestly I don't even care either. Just wanted to make it look similar to other code. Are there other opinions?

PeterKietzmann · 2015-09-29T16:26:03Z

drivers/at30tse75x/at30tse75x.c

+                        AT30TSE75X_CONFIG__RESOLUTION_SHIFT;
+
+        /* Wait until conversion is finished */
+        xtimer_usleep((uint32_t)(25000 << resolution));


I understand that the conversion time is dependent from the resolution. But why shifting 25000?

The conversion time is a multiple of 25ms. See datasheet p.47.

EDIT: Shifting because it's multiplication by 2^resolution.

PeterKietzmann · 2015-09-29T17:08:21Z

@daniel-k thanks again for that PR. Code looks nice overall. Please adapt the discussed changes. After that we should see what the build tests report. Again: Does anyone own that hardware (e.g. ATIO1-XPRO dev board) ? Would be nice if the PR is tested by a second party before merging it.

daniel-k · 2015-09-30T14:37:49Z

@PeterKietzmann
When testing the board again it suddenly not worked anymore. After some (too much) debugging I found out that the mcu is too fast calling driver functions that access I2C successively so that the bus free time of at least 600ns (p.48) is violated and communication fails. I fixed this by prefixing every bus access by the driver with a 1us spin. Considering the already slow operation of I2C (100kHz) this shouldn't hurt too much.

Additionally I rebased on master and squashed everything that has been reviewed already into the first commit, as well as some cleanups and a sanity check of the supplied device address.

PeterKietzmann · 2015-10-01T08:47:28Z

drivers/at30tse75x/at30tse75x.c

+        return -1;
+    }
+
+#if ENABLE_DEBUG


I think you can remove these preprocessor instructions now

PeterKietzmann · 2015-10-01T08:56:44Z

@daniel-k thanks for addressing my comments. I'm fine with the small delay before the bus access. IIRC we had a similar situation for an other sensor (but with the CS line of SPI). Please address my latest minor comments. After that we can run Travis.

daniel-k · 2015-10-01T09:25:30Z

Travis running, but still needs squashing.

PeterKietzmann · 2015-10-02T10:05:53Z

@daniel-k Travis failed for several boards because of a redefinition of MODULE_AT30TSE75X in sc_at30tse75x.c. Please remove that define out of the script. AFAIK this is generated by the makefile.

PeterKietzmann · 2015-10-02T10:07:47Z

sys/shell/commands/sc_at30tse75x.c

+#include <stdlib.h>
+#include <stdbool.h>
+#include "at30tse75x.h"
+#define MODULE_AT30TSE75X


Why did you introduce this in late before last commit? Travis complains about that define. (He complains about redefinition and I think it is initially defined by the makefile system)

This slipped through by accident. Eclipse doesn't know those defines, so auto-completion doesn't work inside unknown #ifdefs.

Run make eclipsesym to create an eclipse symbol xml file that you can
import into your eclipse project. There's a page in the wiki about it.
On Oct 2, 2015 12:17 PM, "Daniel Krebs" [email protected] wrote:

In sys/shell/commands/sc_at30tse75x.c
#3755 (comment):

* @{

* @file

* @brief Provides shell commands to test AT30TSE75x temperature sensor

* @author Daniel Krebs [email protected]

* @}

*/
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <stdbool.h>
+#include "at30tse75x.h"
+#define MODULE_AT30TSE75X

This slipped through by accident. Eclipse doesn't know those defines, so
auto-completion doesn't work inside unknown #ifdefs.

—
Reply to this email directly or view it on GitHub
https://github.com/RIOT-OS/RIOT/pull/3755/files#r41007887.

PeterKietzmann · 2015-10-02T11:16:23Z

Oh no, there are 14 commits :-) ! @daniel-k please squash again. @OlegHahm the code looks fine to me and Travis succeeded. But no one else has the hardware to test. I tend to merge anyway. Ok with you?

daniel-k · 2015-10-02T12:51:38Z

Finally, travis is happy :-D

PeterKietzmann · 2015-10-02T12:54:55Z

As far as I see it, nobody objects merging this PR. Again: I did not test this PR with hardware but the code looks fine now and makes Travis green. Because I'm away for two weeks from now on, I will just merge the PR, so @daniel-k can use the driver in his project in a more comfortable way. Also I do trust him that he'll take care of it, just if it causes some problems (but I assume that won't be the case ;-) )

PeterKietzmann · 2015-10-02T12:55:02Z

So ACK and go

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor

daniel-k · 2015-10-02T13:07:50Z

Also I do trust him that he'll take care of it, just if it causes some problems

Sure! Thanks for merging!

daniel-k added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Sep 1, 2015

daniel-k force-pushed the pr/at30tse75x branch 2 times, most recently from d0e9619 to 51ab79f Compare September 1, 2015 15:23

PeterKietzmann self-assigned this Sep 1, 2015

OlegHahm modified the milestone: Release NEXT MAJOR Sep 2, 2015

PeterKietzmann reviewed Sep 29, 2015
View reviewed changes

daniel-k force-pushed the pr/at30tse75x branch from 0d171a8 to 3b9b21c Compare September 29, 2015 15:52

PeterKietzmann reviewed Sep 29, 2015
View reviewed changes

daniel-k added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Sep 29, 2015

PeterKietzmann reviewed Sep 29, 2015
View reviewed changes

daniel-k force-pushed the pr/at30tse75x branch from 4958eb6 to 367e4b6 Compare September 30, 2015 14:32

PeterKietzmann reviewed Oct 1, 2015
View reviewed changes

drivers/at30tse75x/at30tse75x.c

return -1;

}

#if ENABLE_DEBUG

Copy link

Member

PeterKietzmann Oct 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove these preprocessor instructions now

daniel-k added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable labels Oct 1, 2015

PeterKietzmann reviewed Oct 2, 2015
View reviewed changes

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor

396b76c

daniel-k force-pushed the pr/at30tse75x branch from 9ec3950 to 396b76c Compare October 2, 2015 11:34

PeterKietzmann added a commit that referenced this pull request Oct 2, 2015

Merge pull request #3755 from daniel-k/pr/at30tse75x

10bddd5

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor

PeterKietzmann merged commit 10bddd5 into RIOT-OS:master Oct 2, 2015

daniel-k deleted the pr/at30tse75x branch October 9, 2015 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor #3755

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor #3755

daniel-k commented Sep 1, 2015

PeterKietzmann commented Sep 1, 2015

PeterKietzmann commented Sep 1, 2015

daniel-k commented Sep 1, 2015

daniel-k commented Sep 2, 2015

PeterKietzmann commented Sep 2, 2015

PeterKietzmann Sep 29, 2015

daniel-k Sep 29, 2015

PeterKietzmann Sep 29, 2015

daniel-k Sep 29, 2015

PeterKietzmann Sep 29, 2015

PeterKietzmann Sep 29, 2015

daniel-k Sep 29, 2015

PeterKietzmann commented Sep 29, 2015

daniel-k commented Sep 30, 2015

PeterKietzmann Oct 1, 2015

PeterKietzmann commented Oct 1, 2015

daniel-k commented Oct 1, 2015

PeterKietzmann commented Oct 2, 2015

PeterKietzmann Oct 2, 2015

daniel-k Oct 2, 2015

jnohlgard Oct 2, 2015

PeterKietzmann commented Oct 2, 2015

daniel-k commented Oct 2, 2015

PeterKietzmann commented Oct 2, 2015

PeterKietzmann commented Oct 2, 2015

daniel-k commented Oct 2, 2015



		#include "periph/i2c.h"
		#include "hwtimer.h"

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor #3755

drivers/at30tse75x: add device driver for AT30TSE75x temperature sensor #3755

Conversation

daniel-k commented Sep 1, 2015

PeterKietzmann commented Sep 1, 2015

PeterKietzmann commented Sep 1, 2015

daniel-k commented Sep 1, 2015

daniel-k commented Sep 2, 2015

PeterKietzmann commented Sep 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann commented Sep 29, 2015

daniel-k commented Sep 30, 2015

Choose a reason for hiding this comment

PeterKietzmann commented Oct 1, 2015

daniel-k commented Oct 1, 2015

PeterKietzmann commented Oct 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann commented Oct 2, 2015

daniel-k commented Oct 2, 2015

PeterKietzmann commented Oct 2, 2015

PeterKietzmann commented Oct 2, 2015

daniel-k commented Oct 2, 2015