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

WIP: UUID endian change #128

Merged
merged 2 commits into from
Dec 7, 2015
Merged

WIP: UUID endian change #128

merged 2 commits into from
Dec 7, 2015

Conversation

marcuschangarm
Copy link
Contributor

Reversed internal representation of long UUID in the UUID class to little endian to be consistent with the short UUID, advertisements, and the nordic softdevice.

Added string constructor to UUID for human readable UUID.

Made changes to the services.

see ARMmbed/ble-nrf51822#82

…ttle endian to be consistent with the short UUID.
@marcuschangarm
Copy link
Contributor Author

@rgrover as we discussed

// error abort
break;
} else if (stringUUID[index] == '-') {
// ignore hyphen
Copy link
Contributor

Choose a reason for hiding this comment

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

add a check here to ensure that nibble is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you want to guard against? I was thinking that as long as there are enough hexadecimal characters in the string, we should just ignore the hyphens.

Copy link
Contributor

Choose a reason for hiding this comment

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

the hyphens could come at the wrong place. This constructor currently accepts -X-X-X-XXXXXXXXXXXXXXXX...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and I think we should allow that. Without a proper feedback mechanism the user could easily mistype a single hyphen and not know what went wrong.

@marcuschangarm
Copy link
Contributor Author

@rgrover @pan-

I've made the requested changes.

@rgrover
Copy link
Contributor

rgrover commented Dec 7, 2015

+1
we need to have a separate discussion regarding the UUID constructor. It may be that in its current form, the UUID constructor is misleading (or not explicit enough) regarding the ordering of the bytes.

typedef enum {
MSB,
LSB
} BitOrder_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

just a trivial note: this is byte-order, not bit-order. I'll fix this when I merge this PR.

@rgrover rgrover merged commit 01eaeee into ARMmbed:develop Dec 7, 2015
@rgrover
Copy link
Contributor

rgrover commented Dec 7, 2015

published "ble-2.1.10"
@pan- : applied reverse_copy()
also renamed BitOrder_t to ByteOrder_t

@apalmieriGH
Copy link

@marcuschangarm @pan- @rgrover : guys, I suppose that depending on the byte order used, the shortUUID should be set accordingly. The following is a proposal for the setupLong() method in UUID.h
What is your feeling?

void setupLong(const LongUUIDBytes_t longUUID, ByteOrder_t order = UUID::MSB) {
        type      = UUID_TYPE_LONG;
        if (order == UUID::MSB) {
            // Switch endian. Input is big-endian, internal representation is little endian.
            std::reverse_copy(longUUID, longUUID + LENGTH_OF_LONG_UUID, baseUUID);
            shortUUID = (uint16_t)((baseUUID[3] << 8) | (baseUUID[2]));
        } else {
            std::copy(longUUID, longUUID + LENGTH_OF_LONG_UUID, baseUUID);
            shortUUID = (uint16_t)((baseUUID[13] << 8) | (baseUUID[12]));
        }
    }

@marcuschangarm
Copy link
Contributor Author

@apalmieriGH Isn't that what we are doing already?

https://github.com/ARMmbed/ble/blob/master/ble/UUID.h#L185

@apalmieriGH
Copy link

I think this is fine just for LSB. Shouldn't be

shortUUID = (uint16_t)((baseUUID[3] << 8) | (baseUUID[2]));

for MSB case?

@marcuschangarm
Copy link
Contributor Author

The line is outside the if-else statement, so baseUUID is already LSB.

@apalmieriGH
Copy link

Maybe I'm missing something. But in the "if" (which is also the default) case the baseUUID is supposed in MSB, right?

@marcuschangarm
Copy link
Contributor Author

Yes, and then it gets reversed copied into baseUUID, so after the if-else baseUUID is LSB regardless of input.

@apalmieriGH
Copy link

Ok, then in this case, the shortUUID shoud be always

shortUUID = (uint16_t)((baseUUID[3] << 8) | (baseUUID[2]));

Do you agree?

@marcuschangarm
Copy link
Contributor Author

No, that would give you BBBB when we want AAAA:

human readable UUID in MSB: XXXXAAAA-XXXX-XXXX-XXXX-XXXXBBBBXXXX 

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.

4 participants