drivers/ata8520e: add initial implementation (without netdev but provides Sigfox feature) by aabadie · Pull Request #7087 · RIOT-OS/RIOT

aabadie · 2017-05-22T10:56:58Z

The ATA8520E is a Sigfox communication module from Microchip. It can be controlled from SPI and is available on the arduino-mkrfox1200 (see #7092).

This PR add supports for the following features:

read Atmel and Sigfox versions
read device ID and PAC keys required to register the device in the Sigfox cloud backend
hardware reset
send frame, bit and send/receive frame

I think it can also be possible to add a netdev adaption.

aabadie · 2018-01-22T08:00:07Z

This PR is ready for review, I tested it (with #7093) and it works now.

kYc0o

I've found some minor things. Please also state in the title "without netdev". It might be used in the future, though I really doubt we can use any protocol on top of sigfox (VERY limited bandwidth).

kYc0o · 2018-01-24T15:33:19Z

drivers/ata8520e/ata8520e.c

+#define CSPIN                (dev->params.cs_pin)
+#define INTPIN               (dev->params.int_pin)
+#define RESETPIN             (dev->params.reset_pin)
+#define POWERPIN             (dev->params.power_pin)


Hmmm... all the above defines look a bit weird to me, since I have no knowledge of what is dev. Did you find this style in other drivers?

Yes, these are helper defines, see here for example

kYc0o · 2018-01-24T15:33:38Z

drivers/ata8520e/ata8520e.c

+#define SPI_CS_DELAY         (US_PER_MS)
+#define DELAY_10_MS          (10 * US_PER_MS)   /* 10 ms */
+#define TX_TIMEOUT           (8U)   /* 8 s */
+#define TX_RX_TIMEOUT        (50U)  /* 50 s */


Please align the comments.

aabadie · 2018-01-25T11:02:26Z

Comments addressed @kYc0o

aabadie · 2018-01-25T11:03:37Z

And PR title updated

kYc0o · 2018-01-25T13:58:10Z

Tested (with #7093) ACK.

haukepetersen · 2018-01-26T08:44:54Z

Sorry for mentioning this only now (are scanning through my backlog this morning...): I think there are still some issues to fix in this driver, most of them w.r.t. to debug code. For example in the init function: if debug is enabled, ata8520e_read_sigfox_version() is called, followed by a call to ata8520e_status() which ends with power_off. Now the read version functions ends with _poweroff(), but the status() function does not turn on power again, so this seems broken, right?

Also, the _poweroff() function seems incorrect, shouldn't the command be send before releaseing the power pin?

These are all things that a reviewer should notice when doing a CODE review! So please make sure you watch for these things!

aabadie · 2018-01-26T09:15:06Z

These are all things that a reviewer should notice when doing a CODE review!

Another thing is to look at the datasheet before starting reviewing.

the _poweroff() function seems incorrect

This is what is in the datasheet, see section 2.1.2.5 page 11

but the status() function does not turn on power again, so this seems broken, right?

No because this function is called by other functions that switch the device on before.

aabadie · 2018-01-26T09:32:02Z

BTW, if you see some things that could be changed (e.g remove ata8520e_status from public API, etc). Let me know here and I'll open a PR with the changes.

haukepetersen · 2018-01-26T10:37:52Z

Never mind about the issue in the poweroff() function, you are right and I should have looked into the datasheet. Maybe we can try to comment such kind of 'uncommon' calls so that it is clear this was intended and required by the device? And please remind me in my own PRs :-)

but the status() function does not turn on power again, so this seems broken, right?

No because this function is called by other functions that switch the device on before.

In general yes, but with debug enabled this seems not to be true for line 291. Power is disabled in line 281, and then not enabled agian inside the ata8520e_status() function in line 291. It seems I am missing something here?!

And I forgot to say: nice work with this driver! I am really not just trying to be nitpicky here. I just recently started to stumbled upon many cases where we (me included) merged stuff with more or less obvious flaws, so I am trying to raise the awareness for thorough reviewing a bit...

kYc0o · 2018-01-26T10:39:36Z

These are all things that a reviewer should notice when doing a CODE review! So please make sure you watch for these things!

I accept that there are some things I don't check exhaustively, but here the real mistake is that this review was done with the author sit next to me, so I asked why some things seemed weird and he explained them. Problem: if someone comes afterwards and such questions are not logged here we trigger the situation just happened. I'll make myself sure I log every question when I review something along with the author.

Second, this is a new driver and the users are very limited. I wasn't expecting a perfect implementation when a first version is PRed. If someone uses it and discovers a problem that can be fixed with another PR, and so on. IMHO there's nothing critical here.

And third: this PR adds a new feature which is super useful in RIOT, I was suprised about the simplicity of the driver and how we could send packets (small, yes) and receive them through the Internet with 0 infrastructure on our side! Thus, having this on master seemed awesome to me, with no harm, nothing was broken in RIOT afterwards.

haukepetersen · 2018-01-26T10:59:20Z

@aabadie see #8449 -> better to fix than to complaim, right :-)

@kYc0o I think we have very differing opinions about reviewing...

I accept that there are some things I don't check exhaustively

This is IMHO a problem.

Second, this is a new driver and the users are very limited. I wasn't expecting a perfect implementation when a first version is PRed. If someone uses it and discovers a problem that can be fixed with another PR, and so on. IMHO there's nothing critical here.

Ok, here we seem to have a very different understanding and/or expectation in the group of maintainers. But here is the wrong place for discussion, I will put this on the agenda of the next maintainer meeting.

And third: this PR adds a new feature which is super useful in RIOT, I was suprised about the simplicity of the driver and how we could send packets (small, yes) and receive them through the Internet with 0 infrastructure on our side! Thus, having this on master seemed awesome to me, with no harm, nothing was broken in RIOT afterwards.

I totally agree, this driver is awesome!

kYc0o · 2018-01-26T11:03:33Z

I accept that there are some things I don't check exhaustively

This is IMHO a problem.

I was talking about this PR.

Ok, here we seem to have a very different understanding and/or expectation in the group of maintainers. But here is the wrong place for discussion, I will put this on the agenda of the next maintainer meeting.

Yes, let's discuss it in the right channel.

aabadie added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 22, 2017

aabadie added this to the Release 2017.07 milestone May 22, 2017

aabadie assigned haukepetersen May 22, 2017

aabadie changed the title ~~Driver ata8520~~ drivers/ata8520: add initial implementation May 22, 2017

aabadie mentioned this pull request May 23, 2017

boards/arduino-mkrfox1200: add initial implementation (without SigFox) #7092

Merged

aabadie force-pushed the driver_ata8520 branch from 3bd2a41 to 6d2a5e2 Compare May 23, 2017 09:00

aabadie mentioned this pull request May 23, 2017

boards/arduino-mkrfox1200: add board configuration for ata8520e #7093

Merged

aabadie force-pushed the driver_ata8520 branch from 6d2a5e2 to a70d4b3 Compare May 23, 2017 12:29

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 30, 2017

aabadie force-pushed the driver_ata8520 branch from a70d4b3 to a51814e Compare August 29, 2017 07:59

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 29, 2017

aabadie force-pushed the driver_ata8520 branch from a51814e to d30c74f Compare August 29, 2017 08:17

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

aabadie force-pushed the driver_ata8520 branch 6 times, most recently from 2b1f0d5 to 361c37a Compare January 12, 2018 14:46

smlng modified the milestones: Release 2018.01, Release 2018.04 Jan 18, 2018

aabadie added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 19, 2018

aabadie force-pushed the driver_ata8520 branch from 361c37a to 254607f Compare January 19, 2018 17:26

aabadie changed the title ~~drivers/ata8520: add initial implementation~~ drivers/ata8520e: add initial implementation Jan 21, 2018

aabadie force-pushed the driver_ata8520 branch from b988bee to 646333a Compare January 21, 2018 16:40

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 21, 2018

aabadie changed the title ~~drivers/ata8520e: add initial implementation~~ drivers/ata8520e: add initial implementation (provides SigFox feature) Jan 21, 2018

aabadie force-pushed the driver_ata8520 branch from 646333a to 80a4246 Compare January 21, 2018 16:46

aabadie changed the title ~~drivers/ata8520e: add initial implementation (provides SigFox feature)~~ drivers/ata8520e: add initial implementation (provides Sigfox feature) Jan 21, 2018

aabadie force-pushed the driver_ata8520 branch 4 times, most recently from a3c9df9 to b39c258 Compare January 22, 2018 07:57

kYc0o requested changes Jan 24, 2018

View reviewed changes

aabadie added 2 commits January 25, 2018 12:01

drivers/ata8520e: initial implementation

4bc4c56

tests: add ata8520e test application

11b7e3b

aabadie force-pushed the driver_ata8520 branch from b39c258 to 11b7e3b Compare January 25, 2018 11:02

aabadie changed the title ~~drivers/ata8520e: add initial implementation (provides Sigfox feature)~~ drivers/ata8520e: add initial implementation (without netdev but provides Sigfox feature) Jan 25, 2018

kYc0o approved these changes Jan 25, 2018

View reviewed changes

kYc0o merged commit 972b0ab into RIOT-OS:master Jan 25, 2018

haukepetersen mentioned this pull request Jan 26, 2018

driver/ata8520e: minor doc fixes #8449

Merged

aabadie mentioned this pull request Jan 29, 2018

drivers/ata8520e: internal implementation improvements #8472

Merged

aabadie deleted the driver_ata8520 branch June 8, 2018 20:49

Conversation

aabadie commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Jan 22, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

kYc0o Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

kYc0o Jan 25, 2018

Choose a reason for hiding this comment

Uh oh!

kYc0o Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 25, 2018

Uh oh!

aabadie commented Jan 25, 2018

Uh oh!

kYc0o commented Jan 25, 2018

Uh oh!

haukepetersen commented Jan 26, 2018

Uh oh!

aabadie commented Jan 26, 2018

Uh oh!

aabadie commented Jan 26, 2018

Uh oh!

haukepetersen commented Jan 26, 2018

Uh oh!

kYc0o commented Jan 26, 2018

Uh oh!

haukepetersen commented Jan 26, 2018

Uh oh!

kYc0o commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aabadie commented May 22, 2017 •

edited

Loading

kYc0o commented Jan 26, 2018 •

edited

Loading