Skip to content

Cellular: Implementation of virtual get_ip_address funtion in ublox-api #12215

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
Jan 9, 2020
Merged

Cellular: Implementation of virtual get_ip_address funtion in ublox-api #12215

merged 1 commit into from
Jan 9, 2020

Conversation

mudassar-ublox
Copy link
Contributor

@mudassar-ublox mudassar-ublox commented Jan 8, 2020 •
edited
Loading

Summary of changes

Override virtual get_ip_address funtion in ublox-api for UBLOX targets UBLOX_C030_U201 and UBLOX_C027.

FIx #12102

Impact of changes

Migration actions required

Documentation

Pull request type 

[*] 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 

[] 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

@ciarmcom ciarmcom requested review from a team January 8, 2020 12:00
@ciarmcom
Copy link
Member

ciarmcom commented Jan 8, 2020

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

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2020

cc @pilotak

0xc0170
0xc0170 requested changes Jan 8, 2020
View reviewed changes
features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp
@@ -404,36 +404,95 @@ void UBLOX_AT_CellularStack::clear_socket(CellularSocket *socket)
}
}

#ifndef UBX_MDM_SARA_R41XM
Copy link
Contributor

Choose a reason for hiding this comment

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

why this one is being skipped? Can you add this detail to the commit message (similar is stated in the description - this is implemented for 2 targets as I understood) ?

Copy link
Contributor Author

@mudassar-ublox mudassar-ublox Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, these changes are for UBLOX_C030_U201 and UBLOX_C027. UBLOX_C030_R41XM is skipped as it does not support UPSND command for getting IP address so instead of using these functions it will use parent class functions, get_ip_address.

@mudassar-ublox
Copy link
Contributor Author

@0xc0170 commit updated, Please review now.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2020

CI started

@AnttiKauppila
Copy link

You will soon have "deadcode" with get_ip_address(); #11942

AnttiKauppila
AnttiKauppila reviewed Jan 8, 2020
View reviewed changes
features/cellular/framework/targets/UBLOX/AT/UBLOX_AT_CellularStack.cpp
@@ -404,36 +404,95 @@ void UBLOX_AT_CellularStack::clear_socket(CellularSocket *socket)
}
}

#ifndef UBX_MDM_SARA_R41XM
const char *UBLOX_AT_CellularStack::get_ip_address()
Copy link

Choose a reason for hiding this comment

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

Would it be possible to call get_ip_address(SocketAddress *) function here instead of duplicating the code?
Would be much easier to remove later on.

Copy link

@AnttiKauppila AnttiKauppila Jan 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Something like
`SocketAddress addr;

get_ip_address(&addr);

return _ip; //Set already in above function`

@0xc0170 0xc0170 mentioned this pull request Jan 8, 2020
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2020

As @AnttiKauppila suggested, this should come in before #11942. We will update then 11942 to get it up to date.

@mudassar-ublox would you implement the suggestion above from the review here? We can retrigger CI once done.

@mbed-ci
Copy link

mbed-ci commented Jan 8, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-pytest

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2020

jenkins-ci/mbed-os-ci_cloud-client-pytest

Was restarted, all good now

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 needs: review and removed needs: CI labels Jan 8, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Jan 8, 2020

@mudassar-ublox Let us know if you want to update based on the suggestion. Otherwise this is ready to get in

@mudassar-ublox
Copy link
Contributor Author

@0xc0170 PR updated according to @AriParkkila suggestions.

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 9, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 9, 2020

Test run: SUCCESS

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

@0xc0170 0xc0170 removed the needs: CI label Jan 9, 2020
@0xc0170 0xc0170 merged commit ec3fc67 into ARMmbed:master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UBLOX G350 get local ip fails
5 participants
@mudassar-ublox @ciarmcom @0xc0170 @AnttiKauppila @mbed-ci