Cellular: Fix plmn trace for IAR #10172
Conversation
IAR compiler does not seem to like printing null strings.
| @@ -259,7 +259,7 @@ nsapi_error_t AT_CellularContext::set_blocking(bool blocking) | |||
|
|
|||
| void AT_CellularContext::set_plmn(const char *plmn) | |||
| { | |||
| tr_info("CellularContext plmn %s", plmn); | |||
| tr_info("CellularContext plmn %s", (plmn ? plmn : "NULL")); | |||
There was a problem hiding this comment.
This seems to be regular use, do we need to do this condition for every const char* for trace msg ? Is this a place to fix it, not tr_ module?
@kjbracey-arm what do you think about this bugfix?
There was a problem hiding this comment.
It's definitely needed in any situation where NULL is a valid value. So if set_plmn(NULL) is legal, this is a necessary change. If set_plmn(NULL) is illegal, it's debatable.
It's generally nice if your trace doesn't blow stuff up with illegal values, so it is handy when the C library catches it for you. (Up until the point you port to another library that isn't nice).
But I'm not generally in favour of hiding illegal conditions. If in this case set_plmn(NULL) is illegal, it's helped us just catch that illegal use.
There's some second-order oddity here in the example that they're literally doing "plmn": 0 in the JSON, rather than "plmn": null, which is triggering the passing of 0 in the C code. JSON null would not define the macro.
Trace code can't do this itself - it's not parsing the format string or handling the parameters - they just go straight to a C library printf.
There was a problem hiding this comment.
In this case we also needs to be careful not to break any API. Cellular-example mbed_app.json has had "null" values defined as 0 for long time, so I don't think we want to change the behaviour anymore. Right?
There was a problem hiding this comment.
Aside from the JSON, the actual CellularInterface API is unclear on whether NULL is legal or not for the various config APIs, but I'd guess it's supposed to be, meaning "reset to none" (or default if APN database?). So I think this particular check is required.
|
ci started |
Test run: FAILEDSummary: 1 of 9 test jobs failed Failed test jobs:
|
|
CI restarted, since the buld failure was actually a GitHob 502 timeout |
Test run: SUCCESSSummary: 13 of 13 test jobs passed |
Description
IAR compiler does not seem to like printing null strings. PLMN can be null depending on user configuration.
This is fix for ARMmbed/mbed-os-example-cellular#125.
Pull request type
Reviewers
@AriParkkila @mirelachirica @AnttiKauppila
Release Notes