-
Notifications
You must be signed in to change notification settings - Fork 510
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.
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
Add two-digit mnc diagnostic flag #1804
Conversation
35b4030
to
f739317
Compare
bf0dda4
to
83b509e
Compare
@@ -412,6 +412,15 @@ int SaraNcpClient::queryAndParseAtCops(CellularSignalQuality* qual) { | |||
r = CHECK_PARSER(resp.readResult()); | |||
CHECK_TRUE(r == AtResponse::OK, SYSTEM_ERROR_AT_NOT_OK); | |||
|
|||
// Preserve digit format data | |||
const int mnc_digits = ::strnlen(mobileNetworkCode, 4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number, let's use sizeof(mobileNetworkCode)
here.
Same here https://github.com/particle-iot/device-os/pull/1804/files#diff-13c08dca4df073df121b6a68266e0105R1520
@@ -302,7 +302,14 @@ class NetworkCellularCellGlobalIdentityMobileNetworkCodeDiagnosticData | |||
{ | |||
CellularGlobalIdentity cgi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed this. Why aren't we zeroing out this struct and not setting neither size nor version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Thank you!
@@ -77,7 +77,7 @@ class SaraNcpClient: public CellularNcpClient { | |||
gsm0710::Muxer<particle::Stream, StaticRecursiveMutex> muxer_; | |||
std::unique_ptr<particle::MuxerChannelStream<decltype(muxer_)> > muxerAtStream_; | |||
CellularNetworkConfig netConf_; | |||
CellularGlobalIdentity cgi_; | |||
CellularGlobalIdentity cgi_ = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: = {};
in C++.
@@ -321,7 +328,7 @@ class NetworkCellularCellGlobalIdentityLocationAreaCodeDiagnosticData | |||
|
|||
virtual int get(IntType& val) | |||
{ | |||
CellularGlobalIdentity cgi; | |||
CellularGlobalIdentity cgi{sizeof(CellularGlobalIdentity),CGI_VERSION_LATEST}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicking: spaces.
Same here https://github.com/particle-iot/device-os/pull/1804/files#diff-027356106780d045bc6ae658497e6c92R350
Also perhaps prefer designated initializers:
CellularGlobalIdentity cgi{sizeof(CellularGlobalIdentity),CGI_VERSION_LATEST}; | |
CellularGlobalIdentity cgi {.size = sizeof(CellularGlobalIdentity), .version = CGI_VERSION_LATEST}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer those, but I thought that was only a C
thing. I will switch over.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK it's in C++20 officially, but GCC supported trivial cases since long ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just did a cursory check and I cannot find where designated initialization has made it into the c++ standard.
@avtolstoy Can you please confirm when it was added to the standard and if our compile flags -std=c++14
support it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See message above. Yes, it'll work as long as the designators are specified in the same order as the fields during the declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I truly like the clarity of the syntax, I would prefer to maintain standard compliance and not become compiler dependent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case you should consider removing __attribute__
or any other GNU Extensions as well.
I simply want to improve readability, so you could also consider:
CellularGlobalIdentity cgi{sizeof(CellularGlobalIdentity),CGI_VERSION_LATEST}; | |
CellularGlobalIdentity cgi = {}; | |
cgi.size = sizeof(cgi); | |
cgi.version = CGI_VERSION_LATEST; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point about __attribute__
, seems like we are well committed to GNU at this point. I will follow your first suggestion, and use the designated initializers so we can keep it on one line.
@zfields Please also set the appropriate milestone for this PR. |
@@ -33,6 +35,8 @@ enum __attribute__((__packed__)) CgiFlags | |||
CGI_FLAG_TWO_DIGIT_MNC = 0b00000001, /*!< Indicates two-digit format of Mobile Network Code */ | |||
}; | |||
|
|||
PARTICLE_STATIC_ASSERT(CgiFlags_size, sizeof(enum CgiFlags) == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer typedef enum XXX {} XXX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The two-digit `mnc` diagnostic allows the original `mnc` to be reconstructed accurately.
Problem
Currently, the
mnc
is translated to and stored as a decimal value. According to the specification,40
and040
are not equal values. Currently when using themcc
andmnc
tuple, there does not appear to be a clash. However, maintaining this distinction is important is important to ensure future bugs do not appear in the system.Solution
By sending an indicator as to the original form of the information, we are able to reconstruct the original value in a lossless manner.
Steps to Test
user/tests/accept/hal-api-test-plan.md
Example App
user/tests/app/cellular_global_identity/cellular_global_identity.cpp
Tickets
Completeness