Feature bluetooth traces #14198
Feature bluetooth traces #14198
Conversation
ciarmcom
added
the
release-type: patch
|
@pan-, thank you for your changes. |
|
handbook PR: ARMmbed/mbed-os-5-docs#1397 |
| return string_buffer[idx]; | ||
| } | ||
|
|
||
| const char *to_string(bool v) |
There was a problem hiding this comment.
Quick question for my own personal C/C++ culture :
Compatibility issues with the existing code and bigger memory footprint put aside, would it be okay to use std::string instead of C-style string? Would a std::string_view with C++17 work as well? What are the pros and cons?
Something like:
const std::string to_string(bool v)
{
if (v) {
return "true";
} else {
return "false";
}
}There was a problem hiding this comment.
std::string implies memory allocation of the string buffer. That is something we want to avoid; especially for constant data. std::string_view would be a better but in this case there is two issues:
- It is part of C++17 while Mbed uses C++ 14.
- Mbed trace does not understand
string_viewso we would have to use thedata()member function everywhere.
In the end, these helpers don't return non const or uninitialized memory and their output target mbed_trace. I think it is the right thing to use const char*.
There was a problem hiding this comment.
Thanks for the detailed answer! After posting, I did some experiment and using std::string_view vs cont char* uses the exact same memory footprint. Of course as you pointed out, you'll need the data() member function everywhere you need to pass a C-string, printf() will be okay but you'll get a compiler warning.
I'm wondering if in the code that we write and control, we should use the C-string style or move to std::string_view to keep up with the nice features. The data() function is not that bad and would only be used where we interface with mbed or legacy code.
There was a problem hiding this comment.
We take these decision depending on the benefits and expectations from the user base. The helpers in this PR are internal to the BLE code base and not exposed to our users. They are not meant to be manipulated, return constant value and just meant to be fed into a tr_ trace. In that context, I'm not seeing much benefit for my team to recreate a C++14 version of string_view which main benefit is to be a compatible replacement for std::string and use it just for internal purposes.
On the other hand, when it make sense for the users we don't hesitate to back port standard APIs. For example, mbed::Span was present in mbed OS even before standardization because it just make sense to avoid carrying around the size associated to an array.
|
This pull request has automatically been marked as stale because it has had no recent activity. @noonfom, @ARMmbed/mbed-os-maintainers, please complete review of the changes to move the PR forward. Thank you for your contributions. |
mergify
bot
dismissed
paul-szczepanek-arm’s stale review
Pull request has been modified.
|
@0xc0170 I think the PR is good to go now, is it possible to start CI ? @paul-szczepanek-arm @noonfom Can you confirm with an additional review ? |
|
This PR cannot be merged due to conflicts. Please rebase to resolve them. |
pan-
force-pushed
the
feature-bluetooth-traces
branch
from
0027f8b
to
a5e3297
Compare
|
Rebased. @0xc0170 Do you expect anything more from this PR ? |
mergify
bot
dismissed
paul-szczepanek-arm’s stale review
Pull request has been modified.
paul-szczepanek-arm
added
needs: CI
and removed
needs: work
stale
|
CI started |
Jenkins CI Test : ❌ FAILEDBuild Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mergify
bot
dismissed
paul-szczepanek-arm’s stale review
Pull request has been modified.
|
Fixed cmake, tested build. Ready for rerun. |
|
CI started |
Jenkins CI Test : ✔️ SUCCESSBuild Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & ArtifactsCLICK for Detailed Summary
|
mbedmain
removed
release-type: patch
This commit introduces changes to fix a missing method, `as_entry`, and traces that were not updated properly to work with this new method. Prior to this fix, it was not possible to use KVStoreSecurityDb as the build would fail since the `as_entry` method was not declared in the class header file. This bug was introduced with commit 957486e in PR ARMmbed#14198
This commit introduces changes to fix a missing method, `as_entry`, and traces that were not updated properly to work with this new method. Prior to this fix, it was not possible to use KVStoreSecurityDb as the build would fail since the `as_entry` method was not declared in the class header file. This bug was introduced with commit 957486e in PR ARMmbed#14198
Summary of changes
Addition of traces to the whole BLE stack. This work has been conducted to help debugging of Bluetooth application.
Documentation
Mbed OS documentation has been amended to explain how tracing can be enabled.
Pull request type
Test results
Reviewers
@paul-szczepanek-arm @noonfom