Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: add driver for the AT24MAC unique ID chip #12746

Merged
merged 4 commits into from
Feb 13, 2020

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 19, 2019

Contribution description

The AT24MAC is an EEPROM that provides unique ID functionality.
On one address it provides normal AT24xxx EEPROM operations (#11929), but on a separate I2C address a read-only EUI-48 or EUI-64 and a 128-bit ID are provided.

This adds a simply driver for this chip.

Since the driver has no state and will only read a single, read-only memory location, I chose to depart a bit from the common RIOT driver pattern.

There is no init() function (since nothing needs to be initialized), but the at24mac_get_xxx() functions will take the index in the at24mac_params[] array.
This way it can be directly mapped to the proposed interface indices.

Testing procedure

Run tests/driver_at24mac.

For me this prints

avr-rss2

2019-11-19 23:06:55,956 # EUI-64: fc c2 3d 00 00 00 0b b1
2019-11-19 23:06:55,962 # ID-128: 44 15 85 18 81 90 00 21 78 4b a0 00 a0 00 00 00
2019-11-19 23:06:55,962 # [SUCCESS]

same54-xpro

2020-02-13 14:59:19,770 # EUI-48: fc c2 3d 05 02 e3
2020-02-13 14:59:19,779 # ID-128: 0a 70 08 00 64 10 04 80 5c 92 a0 00 a0 00 00 00
2020-02-13 14:59:19,780 # [SUCCESS]

Issues/PRs references

#12641

@benpicco benpicco added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Nov 19, 2019
@benpicco benpicco requested a review from maribu November 19, 2019 22:12
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 19, 2019
@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Nov 19, 2019
@maribu
Copy link
Member

maribu commented Nov 19, 2019

Hmm. The EUI-48 the test printed for your chip has the group flag set. This seems strange to me.

@benpicco
Copy link
Contributor Author

Uh you are right - and it should also start with fc c2 3d

@benpicco
Copy link
Contributor Author

I should read more carefully: the data sheet says

Factory-programmed EUI-48 or EUI-64 Compatible Address

So AT24MAC402 provides an EUI-48 and AT24MAC602 provides an EUI-64 address.

@herjulf
Copy link
Contributor

herjulf commented Nov 20, 2019

Yes

main(): This is RIOT! (Version: 2020.01-devel-767-g8836f-at24mac)
EUI-64: fc c2 3d 00 00 00 12 60
ID-128: 44 15 85 18 81 90 00 21 54 04 a0 00 20 00 01 00
[SUCCESS]

Reads correct EUI64.
And of course 6LoWPAN and ip6 uses EUI64.

@maribu
Copy link
Member

maribu commented Nov 20, 2019

I think it would be quite possible to have a board with both flavours of EUI chips added. That would be better handled by returning e.g. -ENOTSUP if the wrong kind of EUI is requested.

Maybe it is easier to simply always provide both functions. With the new IS_USED() macro this could be handled quite nicely. Normally we would not expect users to ever request the wrong flavour of EUI, so link time garbage collection will get rid of the overhead for those cases anyway.

@herjulf
Copy link
Contributor

herjulf commented Nov 20, 2019

Would be nice to get rid of the ifdefs yes. From what I see there is no chip id. So we can't see if it's a 48 or 64 chip.
Yes providing both both function should be less of problem. Chip will only be called from drivers that must know what they want.

@herjulf
Copy link
Contributor

herjulf commented Nov 21, 2019

Just a comment. Maribu traced an invalid eiu48 format.
A follow up on that: We could also check for vendor id/OUI.

main(): This is RIOT! (Version: 2020.01-devel-767-g8836f-at24mac)
EUI-48: 3d 00 00 01 81 c6
EUI-64: fc c2 3d 00 00 01 81 c6
ID-128: 44 15 85 18 81 90 00 21 80 dd a0 00 20 00 01 00
[SUCCESS]

We see the eui64 read returns the correct vendor 24 bit OUI
For Atmel: fc c2 3d

@benpicco
Copy link
Contributor Author

Just a comment. Maribu traced an invalid eiu48 format.
A follow up on that: We could also check for vendor id/OUI.
We see the eui64 read returns the correct vendor 24 bit OUI
For Atmel: fc c2 3d

It's not guaranteed the OUI will always be from Atmel. The data sheet even says

For customers with their own IEEE-assigned OUI or Company ID, Atmel offers the time saving option to manage and deliver custom AT24MAC402/602 devices with their EUI-48/64 values uniquely pre-programmed at delivery. Contact your local Atmel Sales Office for additional information.

@herjulf
Copy link
Contributor

herjulf commented Nov 21, 2019

Yes that's correct. Never come across any though. So maybe your first approximation is straight-forward and enough..

@benpicco
Copy link
Contributor Author

Maybe it is easier to simply always provide both functions. With the new IS_USED() macro this could be handled quite nicely.

Instructions unclear, added a type field.

@herjulf
Copy link
Contributor

herjulf commented Nov 26, 2019

I tested and it works! Code looks clean but I'm not qualified with the RIOT machinery yet.
So we stick USEMODULE += at24mac6xx to the BOARD structure plus the PARAMS?

And radio driver can later check what to use for eui?
How about the execution order?

@herjulf
Copy link
Contributor

herjulf commented Nov 29, 2019

Hi,
I've moved at24mac_params.h in this case boards/avr-rss2/include/ as this holds board specific data as the i2c address(es). Or?

One way to provide address to the radio module


diff --git a/drivers/at86rf2xx/at86rf2xx.c b/drivers/at86rf2xx/at86rf2xx.c
index 386b45c..857e401 100644
--- a/drivers/at86rf2xx/at86rf2xx.c
+++ b/drivers/at86rf2xx/at86rf2xx.c
@@ -32,6 +32,7 @@
 #include "at86rf2xx_registers.h"
 #include "at86rf2xx_internal.h"
 #include "at86rf2xx_netdev.h"
+#include "at24mac.h"
 
 #define ENABLE_DEBUG (0)
 #include "debug.h"
@@ -100,11 +101,15 @@ void at86rf2xx_reset(at86rf2xx_t *dev)
         at86rf2xx_set_state(dev, AT86RF2XX_STATE_FORCE_TRX_OFF);
     }
 
+#ifdef MODULE_AT24MAC6XX
+    at24mac_get_eui64(0, &addr_long);
+#else
     /* get an 8-byte unique ID to use as hardware address */
     luid_get(addr_long.uint8, IEEE802154_LONG_ADDRESS_LEN);
     /* make sure we mark the address as non-multicast and not globally unique */
     addr_long.uint8[0] &= ~(0x01);
     addr_long.uint8[0] |=  (0x02);
+#endif
     /* set short and long address */
     at86rf2xx_set_addr_long(dev, &addr_long);
     at86rf2xx_set_addr_short(dev, &addr_long.uint16[ARRAY_SIZE(addr_long.uint16) - 1]);

Simple but has some drawbacks: Adds ifdef's and does not support different instances of at24mac chip. I think we can assume we'll need the eui64 address/call?

Did you have any other ideas? Stuff into luid_get()?


@benpicco benpicco force-pushed the at25mac branch 2 times, most recently from f9cf0f6 to 853a117 Compare December 16, 2019 19:06
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

This PR is in very good shape and apparently it was tested with success.

I have one minor question. Except that, do you see other things missing @benpicco ?

drivers/include/at24mac.h Outdated Show resolved Hide resolved
@benpicco benpicco requested a review from dylad February 13, 2020 12:57
@benpicco
Copy link
Contributor Author

@aabadie This should be good as is.
It will only get useful with #12641, but would already good to have this in to use it as a demonstrator.

btw.: if someone wants to test, same54-xpro comes with a AT24MAC402.

@benpicco benpicco force-pushed the at25mac branch 3 times, most recently from c864784 to 00950fb Compare February 13, 2020 14:01
@dylad
Copy link
Member

dylad commented Feb 13, 2020

After setting the right I2C address, the test was working fine on SAME54-XPRO:

2020-02-13 15:02:45,477 # main(): This is RIOT! (Version: 2020.04-devel-486-gb762bc-at25mac)
2020-02-13 15:02:45,481 # EUI-48: fc c2 3d 0d 9e 29
2020-02-13 15:02:45,487 # ID-128: 0a 70 08 00 64 10 04 80 d0 92 a0 00 a0 00 00 00
2020-02-13 15:02:45,488 # [SUCCESS]

@dylad dylad added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Feb 13, 2020
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Almost good, but there's a remaining nit. You can squash directly.

drivers/Makefile.include Outdated Show resolved Hide resolved
The AT24MAC is an EEPROM that provides unique ID functionality.
On one address it provides normal AT24xxx EEPROM operations, but
on a seperate i2c address a read-only EUI-64 and a 128-bit ID are
provided.

This adds a simply driver for this chip.
Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

All good and tested.

ACK

@dylad dylad added this to the Release 2020.04 milestone Feb 13, 2020
@benpicco benpicco merged commit f9f222c into RIOT-OS:master Feb 13, 2020
@benpicco benpicco deleted the at25mac branch February 13, 2020 15:39
@herjulf
Copy link
Contributor

herjulf commented Feb 13, 2020

Thanks for fixing this.
But it seems the EUI64 chip selection is not correct.
tests/driver_at24mac BOARD=avr-rss2 with EUII64 chip goes for EUI48.

main(): This is RIOT! (Version: 2020.04-devel-501-gda78d)
EUI-48: 3d 00 00 01 83 7e
ID-128: 44 15 85 18 81 90 00 21 24 e7 a0 00 20 00 01 00
[SUCCESS]

Cheers

@benpicco
Copy link
Contributor Author

Huh, that's odd - I'm getting

2020-02-13 22:40:35,252 # main(): This is RIOT! (Version: 2020.04-devel-501-gda78d)
2020-02-13 22:40:35,255 # EUI-64: fc c2 3d 00 00 00 0b b1
2020-02-13 22:40:35,263 # ID-128: 44 15 85 18 81 90 00 21 78 4b a0 00 a0 00 00 00
2020-02-13 22:40:35,263 # [SUCCESS]

@herjulf
Copy link
Contributor

herjulf commented Feb 13, 2020

Hmm you're right. With fresh clone. It's correct.

main(): This is RIOT! (Version: 2020.04-devel-501-gda78d)
EUI-64: fc c2 3d 00 00 01 83 7e
ID-128: 44 15 85 18 81 90 00 21 24 e7 a0 00 20 00 01 00
[SUCCESS]

Thought I could handle git. A fresh pull and no diffs but must have some untracked files playing jokes. Sorry. But happy. BTW the HW EUI64 is not yet reflected i the networking code. Right?

@benpicco
Copy link
Contributor Author

A fresh pull and no diffs but must have some untracked files playing jokes.

Good to hear :)

BTW the HW EUI64 is not yet reflected i the networking code. Right?

Unfortunately not.
We first need a way to assign IDs to network interfaces (if is more than one), else this is getting too messy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants