Skip to content

Cellular: Refactor cellular variable visibilities #12123

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

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Cellular: Refactor cellular variable visibilities #12123

merged 1 commit into from
Jan 3, 2020

Conversation

kivaisan
Copy link
Contributor

@kivaisan kivaisan commented Dec 17, 2019

Summary of changes

Cellular: Refactor cellular variable visibilities

  • Earlier some variables were public even though used only internally
  • Also refactored variables to the end of class definitions
  • Removed duplicate _property_array from CellularDevice
  • Changed _impl methods as protected

Impact of changes

This can potentially be a breaking change but at least currently none of the existing mbed-os cellular targets required any changes.

Migration actions required

If customer wants to use some cellular variable marked as private, they need to inform @ARMmbed/mbed-os-wan and we can review the case.

Documentation


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] 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)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ARMmbed/mbed-os-wan


@ciarmcom ciarmcom requested review from a team December 17, 2019 14:00
@ciarmcom
Copy link
Member

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

@adbridge adbridge requested a review from bulislaw December 18, 2019 15:12
@adbridge
Copy link
Contributor

@bulislaw please review

@adbridge
Copy link
Contributor

@kivaisan What migration actions might people have to take ? Also presumably there will need to be some documentation changes to accompany this ? If so please comment accordingly in the documentation section.

Copy link
Contributor

@adbridge adbridge left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me

@adbridge
Copy link
Contributor

Going to start ci while waiting for @bulislaw review

@mbed-ci
Copy link

mbed-ci commented Dec 18, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@kivaisan
Copy link
Contributor Author

kivaisan commented Dec 19, 2019

@kivaisan What migration actions might people have to take ? Also presumably there will need to be some documentation changes to accompany this ? If so please comment accordingly in the documentation section.

Like mentioned in description, this didn't require any changes to any of our existing cellular targets. If someone uses variable which was originally intented to be internal but was incorrectly set as public earlier, they just need to inform us and we can evaluate whether tre visibility of the variable can be changed or is there something wrong with their targets design. This is how we've been handling these earlier.

@kivaisan
Copy link
Contributor Author

CI failures does not seem to relate this PR. I rebased this PR with the latest mbed-os and at least NRF52840_DK + BG96 (cellular) and K64F (ethernet) DNS tests are OK.

So could you please restart the CI?

@mbed-ci
Copy link

mbed-ci commented Dec 19, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@kivaisan
Copy link
Contributor Author

I found this UT failure but this PR does not cause it. I also verified the branch locally and UTs are OK

13:09:08  /builds/ws/mbed-os-ci_unittests@3/unitTest-4755/mbed-os/UNITTESTS/events/equeue/test_equeue.cpp:680: Failure
13:09:08  Value of: touched > 1
13:09:08    Actual: false
13:09:08  Expected: true

@kjbracey
Copy link
Contributor

Known failure - proposed fix is in #12126. Will retrigger.

@mbed-ci
Copy link

mbed-ci commented Dec 19, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@kivaisan
Copy link
Contributor Author

Again the latest failure is not this PR related.

@adbridge
Copy link
Contributor

@kivaisan looks like we need a rebase

- Earlier some variables were public even though used only internally
- Also refactored variables to the end of class definitions
- Removed duplicate _property_array from CellularDevice
- Changed _impl methods as protected
@kivaisan
Copy link
Contributor Author

Rebased

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 2, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 2, 2020

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_dynamic-memory-usage

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 3, 2020

I restarted dynamic memory usage. There was a failure in the filesystem one, will check other PRs and master

@0xc0170 0xc0170 added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: CI labels Jan 3, 2020
@0xc0170 0xc0170 merged commit 0dbf34b into ARMmbed:master Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING-CHANGE release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants