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

BG96: Add correct get_ip_address implementation #14814

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

boraozgen
Copy link
Contributor

@boraozgen boraozgen commented Jun 22, 2021

Opened again since #14761 was closed unintentionally.

The default get_ip_address implementation was not working for
BG9x. Furthermore the cellular connect routine tries to get the
address multiple times, which added around 2 seconds of
unnecessary delay to the connection. This commit adds the correct
implementation using the AT+QIACT? command.

Summary of changes

Impact of changes

Migration actions required

Documentation


Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[X] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


The default get_ip_address implementation was not working for
BG9x. Furthermore the cellular connect routine tries to get the
address multiple times, which added around 2 seconds of
unnecessary delay to the connection. This commit adds the correct
implementation using the AT+QIACT? command.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Jun 22, 2021
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested a review from a team June 22, 2021 13:00
@0xc0170 0xc0170 requested a review from a team June 22, 2021 13:20
_stack_type = IPV4_STACK;
} else if (context_type == 2) {
_stack_type = IPV6_STACK;
}

Choose a reason for hiding this comment

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

either it's physically impossible for other values to be here or there should be an else clause

Copy link
Contributor Author

@boraozgen boraozgen Jun 23, 2021

Choose a reason for hiding this comment

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

Only allowed values are 1 and 2 according to the BG96 TCP/IP application note.

Choose a reason for hiding this comment

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

in which case the second else if is redundant and could just be an else, would also make my comment below moot as well but it's all nitpicking really

_stack_type = IPV6_STACK;
}

if (_at.read_string(_ip, PDP_IPV6_SIZE) != -1) {

Choose a reason for hiding this comment

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

I guess this should only run if context_type != 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

context_type is always 1 or 2, would only make sense to run if context_state = 1 (activated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW, read_string would return -1 if there is no such string, so I believe it is fine like this. The overridden get_ip_address is also implemented this way.

Copy link
Member

@paul-szczepanek-arm paul-szczepanek-arm left a comment

Choose a reason for hiding this comment

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

The code looks sensible but I cannot test on the board myself, I trust the @boraozgen did.

_stack_type = IPV4_STACK;
} else if (context_type == 2) {
_stack_type = IPV6_STACK;
}

Choose a reason for hiding this comment

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

in which case the second else if is redundant and could just be an else, would also make my comment below moot as well but it's all nitpicking really

@mergify mergify bot added needs: CI and removed needs: review labels Jun 23, 2021
@ciarmcom ciarmcom added the stale Stale Pull Request label Jun 24, 2021
@ciarmcom
Copy link
Member

This pull request has automatically been marked as stale because it has had no recent activity. @ARMmbed/mbed-os-maintainers, please start CI to get the PR merged.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2021

CI started

@0xc0170 0xc0170 removed the stale Stale Pull Request label Jun 24, 2021
@mbed-ci
Copy link

mbed-ci commented Jun 24, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@0xc0170 0xc0170 merged commit 43f1ee7 into ARMmbed:master Jun 24, 2021
@mergify mergify bot removed the ready for merge label Jun 24, 2021
@mbedmain mbedmain added release-version: 6.13.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants