Skip to content

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

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
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
Loading

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 a team and mirelachirica 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
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
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 Outdated
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 Outdated

This comment was marked as outdated.

Sign in to view

@jarvte jarvte force-pushed the drop_bg96_cgact_support branch from af2e395 to 83c18fc Compare February 26, 2019 10:36
mirelachirica
mirelachirica reviewed Feb 26, 2019
View reviewed changes
features/cellular/framework/AT/AT_CellularNetwork.h Outdated
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.

@jarvte jarvte force-pushed the drop_bg96_cgact_support branch from 83c18fc to 048335a Compare February 27, 2019 06:58
@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 jarvte force-pushed the drop_bg96_cgact_support branch from 048335a to 4077898 Compare February 28, 2019 06:36
@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
Loading

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
Loading

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

3 more reviewers

@cmonr cmonr cmonr approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@mirelachirica mirelachirica mirelachirica approved these changes

Reviewers whose approvals may not affect merge requirements

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.

8 participants

@jarvte @ciarmcom @cmonr @bulislaw @mbed-ci @0xc0170 @AriParkkila @mirelachirica