Refactor NimBLEAdvertisementData/NimBLEExtAdvertising #274
Conversation
thekurtovic
commented
Dec 21, 2024
•
thekurtovic
commented
- NimBLEUtils added method to get UUID type from size.
- NimBLEUtils added method to get service data UUID type from size.
- NimBLEUUID added method to get location within payload.
- NimBLEUUID added method as wrapper for NimBLEUtils::getUUIDType().
thekurtovic
force-pushed
the
misc-clean-up
branch
from
67ca9bc to
b431f7f
Compare
|
Thanks! Good idea, I think changing the name to |
thekurtovic
force-pushed
the
misc-clean-up
branch
from
b431f7f to
48cf5f3
Compare
|
Done. |
thekurtovic
force-pushed
the
misc-clean-up
branch
3 times, most recently
from
2164ed0 to
ae270ac
Compare
thekurtovic
changed the title
thekurtovic
force-pushed
the
misc-clean-up
branch
2 times, most recently
from
1d504bf to
133a242
Compare
* NimBLEUtils added method to get UUID type from size. * NimBLEUtils added method to get service data UUID type from size. * NimBLEUUID added method to get location within payload. * NimBLEUUID added method as wrapper for NimBLEUtils::getType().
thekurtovic
force-pushed
the
misc-clean-up
branch
from
133a242 to
c853340
Compare
There was a problem hiding this comment.
I like the consolidation idea, I think another approach may be better though.
I'm thinking these utility functions belong more in the advertising data class and that extended advertising should use or inherit from the class to keep the code all in one place to reduce the duplication.
| return 0; | ||
| } | ||
| } // getServiceType | ||
|
|
There was a problem hiding this comment.
Since the return value of these only relates to and is only used for advertising I feel they would be better placed in the advertising classes.
There was a problem hiding this comment.
I think we should also rename getServiceType to getServiceDataType
| } | ||
| return -1; | ||
| } // getLoc | ||
|
|
There was a problem hiding this comment.
I'm not sure these belong in the uuid class since the type returned is an advertisement uuid type and not the actual uuid type and getLoc probably isnt useful outside of advertising data. Not sure what the use case is as a public API.
There was a problem hiding this comment.
Admittedly this one didn't really make sense to me, but was purely in the pursuit of avoiding duplicate code. I put it in NimBLEUUID since the snippet was accessing accessing a UUID object originally.
Has no business being public really.
|
I had considered the same, and thought that would be more appropriate but didn't want to go down that route in case you had any objections to a big change. |
|
Since this isn't really essential at the moment I think it best to refactor with the larger scale approach so the changes make more sense to the overall classes. Not sure yet if inheritance is the best approach or if the extended advertising class should just have a member variable of |
