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

Jump to bottom

Cellular: CGACT not supported in coming firmware in BG96 #9837

Merged
merged 1 commit into from Mar 14, 2019
Merged

Cellular: CGACT not supported in coming firmware in BG96 #9837

merged 1 commit into from Mar 14, 2019

Conversation

jarvte
Copy link
Contributor

@jarvte jarvte commented Feb 25, 2019 •
edited

Description

Fix issue #9749

Fix is to use BG96 proprietary commands instead of CGACT.
API CellularNetwork::is_active_context changed but it takes default values so not marked as Breaking change.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@mirelachirica

Release Notes

Change API CellularNetwork::is_active_context to take more parameters to reduce copy-paste code.
API can be use in old way as parameters have default values.
Change CGACT to QIACT in case as BG96 module. Reason is that in upcoming firmware versions only QIACT is supported. Current firmware versions do support both commands.

@ciarmcom ciarmcom requested review from mirelachirica and a team February 25, 2019 14:00
@ciarmcom
Copy link
Member

@jarvte, thank you for your changes.
@mirelachirica @ARMmbed/mbed-os-wan @ARMmbed/mbed-os-maintainers please review.

@cmonr
Copy link
Contributor

cmonr commented Feb 25, 2019

@jarvte Mind adding a Release Notes blurb?

@cmonr cmonr added the risk: G label Feb 25, 2019
@jarvte
Copy link
Contributor Author

jarvte commented Feb 26, 2019

@jarvte Mind adding a Release Notes blurb?

Sure, done.

AriParkkila
AriParkkila reviewed Feb 26, 2019
View reviewed changes
Copy link

@AriParkkila AriParkkila left a comment

Choose a reason for hiding this comment

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

All functions could return nsapi_error_t?
AT clear error should be handled by caller and not deactivate_contexts?

mirelachirica
mirelachirica reviewed Feb 26, 2019
View reviewed changes
features/cellular/framework/API/CellularNetwork.h Outdated
*
* @return true is any context is active, false otherwise or in case of error
* @param number_of_active_contexts If given then in return contains the number of active contents
* @param cid If given then active contents are checked only against this cid
Copy link
Contributor

Choose a reason for hiding this comment

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

contents -> contexts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

features/cellular/framework/API/CellularNetwork.h Outdated
* @param number_of_active_contexts If given then in return contains the number of active contents
* @param cid If given then active contents are checked only against this cid
*
* @return true is any (or the given cid) context is active, false otherwise or in case of error
Copy link
Contributor

Choose a reason for hiding this comment

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

is -> if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96_CellularNetwork.cpp
bool active_found = false;
int context_id;
int active;
// read active contexts
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this "QIACT" part be overritten and rest of the code be in base class since it is so much and is copy pasted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, fixed.

features/cellular/framework/targets/QUECTEL/BG96/QUECTEL_BG96_CellularNetwork.cpp
bool active_found = false;
int context_id;
int active;
// read active contexts

This comment was marked as outdated.

Sign in to view

mirelachirica
mirelachirica reviewed Feb 26, 2019
View reviewed changes
features/cellular/framework/AT/AT_CellularNetwork.h Outdated
* same response. Can be overridden by the target class.
*
*/
virtual void set_context_command();
Copy link
Contributor

Choose a reason for hiding this comment

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

would get_context_state_command() be better name and comment:
"... Sends command to query the active state of the PDP contexts... Can be overridden by the target class."

Copy link
Contributor

Choose a reason for hiding this comment

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

@jarvte ^^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed as suggested

@bulislaw
Copy link
Member

This PR is at risk of missing 5.12 release as it's marked as "needs: work". Code freeze is coming! On Friday 1st. Please made necessary updates ASAP and make sure the reviewers are aligned for prompt code inspection.

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 27, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@cmonr
Copy link
Contributor

cmonr commented Feb 27, 2019

@jarvte Please take a look at the failed unit tests.

@jarvte
Copy link
Contributor Author

jarvte commented Feb 28, 2019

@jarvte Please take a look at the failed unit tests.

Fixed. It was the last function name change...

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2019

Can this be postponed to 5.12.1? We got high number of feature PR still opened for 5.12.

@jarvte
Copy link
Contributor Author

jarvte commented Feb 28, 2019 •
edited

Can this be postponed to 5.12.1? We got high number of feature PR still opened for 5.12.

From my point of view yes. I don't know about the person who wrote the issue #9749

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 28, 2019 •
edited

Thanks ! It might still get in earlier but rather we focus on critical features/fixes now at this stage for 5.12.0

@cmonr cmonr removed the risk: A label Feb 28, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 8, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 9, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@0xc0170 0xc0170 merged commit cf76b74 into ARMmbed:master Mar 14, 2019
@jarvte jarvte deleted the drop_bg96_cgact_support branch March 29, 2019 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@mirelachirica mirelachirica mirelachirica approved these changes

Assignees
No one assigned
Labels
release-version: 5.12.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jarvte @ciarmcom @cmonr @bulislaw @mbed-ci @0xc0170 @AriParkkila @mirelachirica