Skip to content

Used a macro for UUID generation, switched from C-style casts to reinterpret_cast, renamed callback#359

Merged
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
Avamander:patch-2
Jun 12, 2021
Merged

Used a macro for UUID generation, switched from C-style casts to reinterpret_cast, renamed callback#359
JF002 merged 2 commits intoInfiniTimeOrg:developfrom
Avamander:patch-2

Conversation

@Avamander
Copy link
Copy Markdown
Collaborator

@Avamander Avamander commented May 16, 2021

I tried to separate these changes a bit more, but couldn't.

The PR makes some minor improvements to the music watchapp:

  1. Switches to a macro for UUID generation. I think this might save some RAM+ROM, but primarily gets rid of all these hard-to-read assignments in the constructor.
  2. Switches from C-style casts to reinterpret_cast
  3. To avoid a name conflict, renames MSCallback to MusicCallback, also did a minor variable inlining (that the compiler will also do above -O0 very likely)
  4. const-ified the value getters for more safety
  5. Moved a few functions around a tiny bit to group getters

(It should remain functionally identical, at least my tests with GB didn't show any differences)

Let me know if I should strip out some of these changes.

I would also really like to know if the macro is okay.

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Jun 12, 2021

Looks good to me!

2 small details:
I think MusicService::characteristicDefinition could be initialized in the initialization list instead of in the core of the ctor() like it's done in other services (ex : https://github.com/JF002/InfiniTime/blob/develop/src/components/ble/DfuService.cpp#L35).

The macro is fine. But it would be even better if we find a way to write a constexpr method to replace the macro :)

@Avamander
Copy link
Copy Markdown
Collaborator Author

I think MusicService::characteristicDefinition could be initialized in the initialization list instead of in the core of the ctor() like it's done in other services (ex : https://github.com/JF002/InfiniTime/blob/develop/src/components/ble/DfuService.cpp#L35).

I'd see if it can be done in a future PR, if you wouldn't mind?

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Jun 12, 2021

I'd see if it can be done in a future PR, if you wouldn't mind?

Yes, of course :)

@JF002 JF002 merged commit a1b3537 into InfiniTimeOrg:develop Jun 12, 2021
@jonvmey
Copy link
Copy Markdown
Contributor

jonvmey commented Jun 12, 2021

The ble_uuid128_t intializers can be replaced with a constexpr function that returns a ble_uuid128_t which eliminates the need for the macros. This has the additional benefit of allowing all of those members to be declared constepxr. I tried making it a member function function at one point but the compiler then complains about trying to call a method of a class before its definition is complete. It worked fine as a free function though.

Also the reinterpret_cast<ble_uuid_t*>s wouldn't be necessary if you changed (&msSomeUuid) to (&msSomeUuid.u) since ble_uuid128_t's first member u is a ble_uuid_t.

I can open a new PR with these changes if you're interested. I was going to suggest them as possibilities before the PR got merged.

@Avamander Avamander deleted the patch-2 branch June 12, 2021 14:29
@Avamander
Copy link
Copy Markdown
Collaborator Author

@jonvmey I would certainly like to see those changes. This PR was intended from the start to be a small incremental cleanup and I would certainly like to get rid of those reinterpret_casts.

@jonvmey jonvmey mentioned this pull request Jun 13, 2021
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.

3 participants