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/802154: improve setting of default short address #5461

Merged
merged 1 commit into from
May 31, 2016

Conversation

PeterKietzmann
Copy link
Member

In #5457 we concluded to spend some more entropy for the default short address generation of 802.15.4 transceivers. As there is no standard for such generation, I propose two possibilities here, demonstrated at the current kw2xrf driver. The first commit takes all CPUID_LEN bytes of the CPUID into account whereas the second commit only uses CPUID_LEN - IEEE802154_LONG_ADDRESS_LEN bytes of the CPUID.

@PeterKietzmann PeterKietzmann added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Area: drivers Area: Device drivers labels May 25, 2016
@PeterKietzmann PeterKietzmann added this to the Release 2016.07 milestone May 25, 2016
@PeterKietzmann
Copy link
Member Author

It took my some minutes to find out you changed your nickname :-)

@@ -438,6 +439,7 @@ int kw2xrf_init(kw2xrf_t *dev, spi_t spi, spi_speed_t spi_speed,
#if CPUID_LEN > IEEE802154_LONG_ADDRESS_LEN
for (int i = IEEE802154_LONG_ADDRESS_LEN; i < CPUID_LEN; i++) {
cpuid[i & 0x07] ^= cpuid[i];
addr_short ^= (uint16_t)(cpuid[i] << ((i & 0x01) * 8));
Copy link
Member

Choose a reason for hiding this comment

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

Just for understanding of that line: you are taking every odd byte of the CPU ID and XOR it with the most-significant byte of the short address and every even byte with the least-significant byte. Is that correct? (maybe add a comment?)

@miri64
Copy link
Member

miri64 commented May 25, 2016

It took my some minutes to find out you changed your nickname :-)

;-)

Other then the readability of the code I'm fine with this change.

@PeterKietzmann
Copy link
Member Author

That is correct. I can give it a comment then. Generally, do you prefer the version in the first commit which takes the full CPUID_LEN into account but introduces an additional loop or do you think the second version (both commits together) which just uses CPUID_LEN - IEEE802154_LONG_ADDRESS_LEN bytes of the CPUID? I personally prefer version 1.

Will adapt this PR once #5461 is clarified.

@miri64
Copy link
Member

miri64 commented May 25, 2016

I think I prefer version 1, too, though it should be CPUID_LEN > 0 (for LONG it should also be <= and some alternative path must be found when the CPUID_LEN is shorter). Rational: if its shorter then or equal to 2 (the current condition for excluding this code; and I know that a vendor will most likely never set such a CPUID, but let's say we emulate the driver for native (where the CPUID is the PID)) the CPUID can still be used as a scalar value (which the loop ensures regardless).

@PeterKietzmann PeterKietzmann force-pushed the adapt_defaul_short_address branch from 740e010 to 20c809b Compare May 27, 2016 09:23
@PeterKietzmann
Copy link
Member Author

@miri64, I moved the short address generation some lines upwards so it will be set once CPUID is not zero. I didn't change the condition for setting the long address because it should be ok for all Kinetis CPUs.

Let's see this PR as a quick fix. I don't want to elaborate on this single driver any more because (i) after discussing in #5457 I think we need some "major " restructuring in terms of address generation and (ii) @jfischer-phytec-iot already proposed a (WIP) driver for netdev2.

What do you say, can we merge as is (after squashing)?

@miri64
Copy link
Member

miri64 commented May 31, 2016

Yes, ACK. Please squash.

@PeterKietzmann PeterKietzmann force-pushed the adapt_defaul_short_address branch from 20c809b to ed470e1 Compare May 31, 2016 11:13
@PeterKietzmann PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 31, 2016
@PeterKietzmann
Copy link
Member Author

Did

@miri64 miri64 merged commit a6df844 into RIOT-OS:master May 31, 2016
@PeterKietzmann PeterKietzmann deleted the adapt_defaul_short_address branch September 2, 2016 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants