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.

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

Integrate cellular network vitals data into `DESCRIBE_x` message #1759

Merged
merged 23 commits into from May 16, 2019

Conversation

@zfields
Copy link
Contributor

commented Apr 29, 2019

ch29061/cellular vitals


  • [enhancement] Integrate cellular network vitals data into DESCRIBE_x message #1759

@zfields zfields changed the base branch from develop to release/v1.2.0 Apr 29, 2019

int r = CHECK_PARSER_URC(reader->scanf("+CREG: %d,%d", &val[0], &val[1]));
int val[4];
// Parse as direct response (ignoring mode)
int r = CHECK_PARSER_URC(reader->scanf("+CREG: %*d,%d,\"%x\",\"%x\",%d", &val[0], &val[1], &val[2], &val[3]));

This comment has been minimized.

Copy link
@technobly

technobly Apr 29, 2019

Member

Looking at this more, <stat> would be val[1] for this direct response with verbose output <n>=2, whereas <stat> would be val[0] for the URC.

This comment has been minimized.

Copy link
@zfields

zfields Apr 29, 2019

Author Contributor

I'm specifically ignoring <mode> with %*d, so val[0] must always be <stat>. If this is a URC it won't parse, and it will have to be reparsed below.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

I have also reduced the tolerance for matching, so this should only parse the appropriate values.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

@technobly Would you review the new logic and see if it addresses your concerns?

@zfields zfields force-pushed the ch29061/cellular-vitals branch from d148529 to 66e4438 Apr 30, 2019

@zfields zfields requested a review from avtolstoy Apr 30, 2019

hal/inc/hal_cellular_global_identity.h Outdated Show resolved Hide resolved
hal/inc/net_hal.h Outdated Show resolved Hide resolved
hal/src/boron/cellular_hal.cpp Show resolved Hide resolved

typedef struct
{
uint16_t mobile_country_code;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

version and/or size for possible future expansion perhaps?

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

The fields is this struct are defined by the 3GPP. They are akin to a phone number <country_code>-<area_code>-<prefix>-<customer>.

I don't expect it to change too much, but I will check the web to see if a version is applied to these fields and sizes.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

This was a suggestion for ABI stability as we do for most of the structs nowadays in case we ever want to extend them and not in the sense that there are multiple variants defined by 3GPP.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Actually, I would insist on this change.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

How big should the version member be and what should the number scheme look like?

This comment has been minimized.

Copy link
@zfields

zfields May 13, 2019

Author Contributor

In the current iteration we have a schema field for describing the data format, as well as a size field.
At this time, I think we should choose a single 16-bit field. It would allow for better packing of the data, which would provide better space efficiency as well as faster data access.
As it stands, we are currently wasting 24-bits, and force aligning the schema field.

}

// Load cached data into result struct
else if (client->getCellularGlobalIdentity(&cgi))

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Nitpicking: Let's call this explicitly and not within an else if block, which will improve readability.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

I don't mind to change, but I'll share the motive behind the style...

This is an error waterfall pattern, and it allows you to achieve a similar pattern to your CHECK() macros.

It prevents deep conditional nesting while providing blocks to validate parameters and calls. It also keeps the functional logic of your method (the only part people care about) in the else at the end.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

I personally first and foremost find it's difficult to comprehend when glancing over the code (as I did during the review), notwithstanding the fact that this can be considered an antipattern in C++ specifically considering RAII and even in C a simple goto would make this code more readable.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

Actually, I want to push back on this. This follows the exact same pattern as the CHECK macro, except that you can choose how you want to handle your error and it's fully transparent.


return SYSTEM_ERROR_NONE;
}
} g_networkCellularCellGlobalIdentityMobileCountryCodeDiagnosticData;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Let's follow the style/pattern the rest of the file does. These classes should also be available only on platforms that define HAL_PLATFORM_CELLULAR to 1. The build currently fails for WiFi devices.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

Actually, since there are so few others (4, maybe 5), I think it may be better to change the rest of the file to follow this.

It keeps each message as a single block and allows for them to be organized much better. For instance, the block makes it easier to wrap such messages in a platform flag (i.e. HAL_PLATFORM_CELLULAR).

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

No strong preference, but again I personally find it's easy to miss the instantiation 🤷‍♂

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Also I remember mentioning this previously, but this source is only being compiled for Gen 2. Ideally your PR should also decouple these diagnostic classes into a separate source file to be available on both Gen 2 and Gen 3.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

Thank you for reiterating; I forgot. We discussed a lot of topics that day...

Do you have a suggestion as to where it should live?

This comment has been minimized.

Copy link
@zfields

zfields May 13, 2019

Author Contributor

This has now been split into system_network_diagnostics.cpp

// Parse as direct response (ignoring mode)
int r = CHECK_PARSER_URC(reader->scanf("+CREG: %*d,%d,\"%x\",\"%x\",%d", &val[0], &val[1], &val[2], &val[3]));
// Reparse as URC
if (r <= 1) { r = CHECK_PARSER_URC(reader->scanf("+CREG: %d,\"%x\",\"%x\",%d", &val[0], &val[1], &val[2], &val[3])); }

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Formatting. Let's not put code within the codeblock on the same line.

This if will invalidate the correct result of the previous scanf for +CREG: <n>,<stat> responses, causing stat to be parsed as <n> instead in the second scanf.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

Forgot to apply the auto formatting. Fixed

// Parse as direct response (ignoring mode)
int r = CHECK_PARSER_URC(reader->scanf("+CGREG: %*d,%d,\"%x\",\"%x\",%d", &val[0], &val[1], &val[2], &val[3]));
// Reparse as URC
if (r <= 1) { r = CHECK_PARSER_URC(reader->scanf("+CGREG: %d,\"%x\",\"%x\",%d", &val[0], &val[1], &val[2], &val[3])); }

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Formatting. Let's not put code within the codeblock on the same line.

This if will invalidate the correct result of the previous scanf for +CREG: <n>,<stat> responses, causing stat to be parsed as <n> instead in the second scanf.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

I forgot to auto format code here, so it will be broken out into multiple lines as soon as I do.

Since, I'm ignoring the first integer, I think the first attempt will not actually parse any values from the URC, so I believe the correct thing to do is check if (0 == r) and that should alleviate your concern.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

That should probably work, yes. It's not a concern though, this is a breaking change bug.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

What is "this"? The current behavior of the UrcParser, or checking for r==0?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

Currently checking for r <= 1 will introduce a bug where +CREG: <n>,<stat> responses would get reparsed as URC, which has a different format (+CREG: <stat>,...) and later we will be erroneously checking <n> instead of <stat>.

This comment has been minimized.

Copy link
@zfields

zfields Apr 30, 2019

Author Contributor

This is all fixed by testing against 0 == r, as we discussed, correct?

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy Apr 30, 2019

Member

That should probably work, yes.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

please confirm the new logic

hal/src/boron/network/sara_ncp_client.cpp Outdated Show resolved Hide resolved
hal/src/electron/modem/mdm_hal.cpp Outdated Show resolved Hide resolved

@zfields zfields force-pushed the ch29061/cellular-vitals branch from 9adbf75 to 001e3f8 Apr 30, 2019

@technobly technobly force-pushed the release/v1.2.0 branch from 054df97 to 9cf3e80 May 2, 2019

@zfields zfields force-pushed the ch29061/cellular-vitals branch 3 times, most recently from 3c9a380 to f738f0b May 2, 2019

@technobly technobly added this to the 1.2.0-rc.1 milestone May 10, 2019

@zfields zfields removed the do not merge label May 10, 2019

@zfields zfields force-pushed the ch29061/cellular-vitals branch from 3e2f1d6 to de84d9e May 14, 2019

uint16_t mobile_network_code;
uint16_t location_area_code;
uint32_t cell_id;
uint8_t schema;

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

What is schema? We sometimes add a version field instead or along with size, but that goes at the top of the structure. Fields that are used for alignment are usually called reservedX, but if this field is removed your structure is already aligned by sizeof(uintptr_t) (12 bytes). If you want to add a reserved field, I'd prefer uint32_t reserved instead.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

hal/inc/net_hal.h Show resolved Hide resolved
@@ -221,6 +223,41 @@ int cellular_credentials_clear(void* reserved) {
return 0;
}

cellular_result_t cellular_global_identity(CellularGlobalIdentity* cgi_, void* reserved_)
{

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

Nitpicking: Please fix the formatting to follow the same style as the rest of the file. Again, I understand we don't have a complete .clang-format, but at the moment let's manually try to follow the style of the file we are modifying.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

done.

hal/src/boron/cellular_hal.cpp Show resolved Hide resolved
hal/inc/net_hal.h Outdated Show resolved Hide resolved
result = SYSTEM_ERROR_INVALID_ARGUMENT;
}
// Load cached data into result struct
else if ((result = client->getCellularGlobalIdentity(&cgi))) {

This comment has been minimized.

Copy link
@avtolstoy
int val[2];
int r = CHECK_PARSER_URC(reader->scanf("+CREG: %d,%d", &val[0], &val[1]));
int val[4] = {-1,-1,-1,-1};
char at_response[64]{};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

This should be char atResponse[64] = {}. Initializer list after = and snake_case should not be used in a camelCase source.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

int val[2];
int r = CHECK_PARSER_URC(reader->scanf("+CGREG: %d,%d", &val[0], &val[1]));
int val[4] = {-1,-1,-1,-1};
char at_response[64]{};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

Same here.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

int val[2];
int r = CHECK_PARSER_URC(reader->scanf("+CEREG: %d,%d", &val[0], &val[1]));
int val[4] = {-1,-1,-1,-1};
char at_response[64]{};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

Same here.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

CHECK(checkParser());
int SaraNcpClient::queryAndParseAtCops(CellularSignalQuality* qual) {
int act;
char mobile_country_code[4] = {};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

snake_case in camelCase source.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

hal/src/electron/cellular_hal.cpp Show resolved Hide resolved
hal/src/electron/cellular_hal.cpp Outdated Show resolved Hide resolved
@@ -0,0 +1,355 @@
/**
******************************************************************************
Copyright (c) 2013-2015 Particle Industries, Inc. All rights reserved.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

Let's use the current copyright header for new files.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

updated

*/

// Mesh signal strength/quality have not been implemented
#if (PLATFORM_ID != 14) && (PLATFORM_ID != 24)

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 14, 2019

Member

Let's prefer to use the macros from platforms.h.

We could alternatively change the return values in https://github.com/particle-iot/device-os/blob/develop/wiring/inc/spark_wiring_mesh.h#L59 to negative for now to cause an error to be returned but I agree that it's probably better not to send them at all.

This comment has been minimized.

Copy link
@zfields

zfields May 14, 2019

Author Contributor

Great catch!

@avtolstoy

This comment has been minimized.

Copy link
Member

commented May 14, 2019

👍
I'm going to double check the scanfs just in case and run a test before approving this, but I'd also prefer some of the comments to be addressed as well.

@zfields

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Pushing refactored code. I have adopted several of the formatting concerns as well as the initialization syntax.

As a heads up, I'm leaving the comments between code blocks in place, but I have attempted to address "why" I have elected to use them as such.

int val[2];
int r = CHECK_PARSER_URC(reader->scanf("+CREG: %d,%d", &val[0], &val[1]));
int val[4] = {-1,-1,-1,-1};
char at_response[64] = {0};

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

snake_case in camelCase file, 0 is unnecessary, {} is enough. I think this is the third time I'm complaining about snake_case in exactly the same place :)

This comment has been minimized.

Copy link
@zfields

zfields May 15, 2019

Author Contributor

You finally pierced my skull! snake_case is now camelCase!

#include "platforms.h"

// Mesh signal strength/quality have not been implemented
#if (PLATFORM_ID != PLATFORM_XENON) && (PLATFORM_ID != PLATFORM_XENON_SOM)

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

FYI we've renamed them in #1774, so after 1.2.0 branch is rebased on develop and this branch is rebase on 1.2.0, this will need to get fixed.

This comment has been minimized.

Copy link
@zfields

zfields May 15, 2019

Author Contributor

updated

*/
typedef struct __attribute__((__packed__))
{
uint16_t size; /*!< \c size is specified by the user application, to inform the device-os of the

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

Consider aligning comments.

This comment has been minimized.

Copy link
@zfields

zfields May 15, 2019

Author Contributor

I used the .clang-format file after typing my comments and this is the output.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

We are all aware that our current .clang-format needs to be updated, because it was merely a PoC done by Julien a long time ago and doesn't cover all the cases, but it shouldn't mean that we completely forget about formatting and blame everything on clang-format.

This comment has been minimized.

Copy link
@avtolstoy

avtolstoy May 15, 2019

Member

And all I'm saying is consider.

This comment has been minimized.

Copy link
@zfields

zfields May 15, 2019

Author Contributor

The current directive is to apply the .clang-format to format the entire file you are working on, make a separate commit and start making your changes. Personally, I don't value the beauty of the code as much as the time spent to make it look that way.

I understand .clang-format is currently incomplete and I don't wish to be confrontational, but if we keep following the routine above, we will have homogeneous code that is capable of evolving over time (as the .clang-format file evolves).

This comment has been minimized.

Copy link
@technobly

technobly May 15, 2019

Member

Good luck getting .clang-format to align these comments the way you want... I agree over time .clang-format used blindly is going to de-beautify areas of the code and make them harder to read. Consider these two options:

  1. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code
    // clang-format off
    code or comments not to format
    // clang-format on
  1. Further down from above link: AlignTrailingComments (bool)

This comment has been minimized.

Copy link
@technobly

technobly May 15, 2019

Member

AlignTrailingComments: true appears to already be set... so I wonder why it's not working.

@avtolstoy

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I'm still seeing textual operator representation on U270 in AT+COPS? response.

[ Modem::getSignalStrength ] = = = = = = = = = =
   119.905 AT send      10 "AT+COPS?\r\n"
   119.906 AT read  +   18 "\r\n+UUSORF: 0,105\r\n"
Socket 0: handle 0 has 105 bytes pending
   119.938 AT read UNK   9 "AT+COPS?\r"
   119.939 AT read  +   26 "\r\n+COPS: 0,0,\"Beeline\",2\r\n"
   119.940 AT read OK    6 "\r\nOK\r\n"
   119.940 AT send       8 "AT+CSQ\r\n"
   119.945 AT read UNK   7 "AT+CSQ\r"
   119.946 AT read  +   14 "\r\n+CSQ: 20,2\r\n"
   119.947 AT read OK    6 "\r\nOK\r\n"
@avtolstoy

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I suspect we may have to either check <format> returned by AT+COPS? and reset it to numeric and rerun AT+COPS? or (which seems somewhat simpler) to always run AT+COPS=3,2 before AT+COPS?, but I remember AT+COPS=3,2 wasn't working correctly on some particular modem. 😞

@zfields zfields force-pushed the ch29061/cellular-vitals branch from 45fbd6b to 709a818 May 16, 2019

@zfields zfields merged commit 0ce2594 into develop May 16, 2019

1 check passed

continuous-integration/travis-ci/push The Travis CI build passed
Details

@m-mcgowan m-mcgowan deleted the ch29061/cellular-vitals branch May 16, 2019

@technobly technobly removed their request for review May 16, 2019

@perotom

This comment has been minimized.

Copy link

commented on 1d80313 May 29, 2019

With this change it is impossible to get the operator name in alphanumeric. I don't see a reason why it should be numeric. I think a lot of people do need the short alphanumeric representation rather than just numeric. Maybe set it to 1 (short format alphanumeric ) instead of 2?

This comment has been minimized.

Copy link
Contributor

replied May 29, 2019

Hey @perotom thanks for your feedback. This change is related to some enhancements to our Device Vitals feature — namely added cellular-specific vitals to the payload we currently send to the Device Cloud. For a variety of reasons related to performance and minimizing data usage, we have made the decision to convert MNC/MCC to a network operator string cloud-side for this feature.

Note that you are still able to use the Cellular.command() API in order to get the operator string in your device firmware app if you need access to it in firmware. Specifically, you'd send AT+COPS=3,1;+COPS? as an argument to that method.

Hope this helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.