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.

Sign up for GitHub

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

Jump to bottom

Cellular: Fix plmn trace for IAR #10172

Merged
merged 1 commit into from
Mar 21, 2019
Merged

Cellular: Fix plmn trace for IAR #10172

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion features/cellular/framework/AT/AT_CellularContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Contributor

@0xc0170 0xc0170 Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

_device->set_plmn(plmn);
}

Expand Down