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

Add target_hash_serial_number() #119

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add target_hash_serial_number() #119

wants to merge 7 commits into from

Conversation

mmoskal
Copy link
Contributor

@mmoskal mmoskal commented Apr 3, 2020

It is to be used by target_get_serial().

It uses FNV-1a for lower half of the serial and murmur2 for the upper half. That should give reasonably good distribution.

Example on SAMD21 (yes, words of serial are not all adjacent).

uint64_t target_get_serial()
{
    static uint64_t cache;
    if (!cache)
    {
        uint32_t serial[4];
        serial[0] = *(uint32_t *)0x0080A00C;
        memcpy(serial + 1, (void *)0x0080A040, 3 * 4);
        cache = target_hash_serial_number(serial, 4);
    }
    return cache;
}

(the current version on D21, D51 is broken, hashing on STM32 is questionable; NRF52 has 64 bit serial, but I would be inclined to hash it anyway)

It is to be used by target_get_serial()
@mmoskal mmoskal requested review from jamesadevine and finneyj April 3, 2020 04:47
@jamesadevine
Copy link
Contributor

The api for getting the hashed serial seems a bit uncouth... Could we instead have two variants? One that returns the "true" serial number, and one that returns the hashed version?

That's just a small inversion to the code you proposed. Instead it would be:

uint64_t target_hashed_serial()
{
     uint64_t serial = target_get_serial();
     uint32_t w1 = hash_fnv1a(&serial, 2 << 2);
     uint32_t w0 = murmur_hash2_core((uint32_t *)&serial, 2);
     w0 &= ~0x02000000; // clear "universal" bit
     return ((uint64_t)w0 << 32 | w1);
}

@mmoskal
Copy link
Contributor Author

mmoskal commented Apr 3, 2020

Well, the api for real serial would need to return a buffer then.

Given that all codal_target_hal.cpp files will need to be updated, I'll do some cleanup there.

@jamesadevine
Copy link
Contributor

Return a buffer because there is no standard length for mcu identifiers?

Yes, in that case it may make sense to return a ManagedBuffer for the full serial number, and we use the hashing mechanism to coerce any serial number into 64-bits.

@jamesadevine
Copy link
Contributor

I don't think it makes sense to change the api for target_get_serial. As you proposed, we should change the underlying implementation of target_get_serial to become a hash, and supply an alternative api for the full N-bit identifier.

target_get_serial should perhaps get the true serial number from this new api?

@mmoskal
Copy link
Contributor Author

mmoskal commented Apr 3, 2020

OK, added new API. Also added default implementation of target_* functions for arm, so we can stop repeating that across all targets. The codal_target_hal.cpp in SAMD21 is now:

#include "codal_target_hal.h"

void target_reset()
{
    NVIC_SystemReset();
}

unsigned target_get_serial_buffer(void *dst, unsigned maxbytes)
{
    if (maxbytes >= 20)
    {
        uint32_t *serial = (uint32_t *)dst;
        // random number to avoid collisions between serial numbers of different manufacturers
        serial[0] = 0xf8495164;
        serial[1] = *(uint32_t *)0x0080A00C;
        memcpy(serial + 2, (void *)0x0080A040, 3 * 4);
    }
    return 20;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants