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.

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

Fix Lora timer cancellation #14422

Merged
merged 4 commits into from
Mar 12, 2021
Merged

Fix Lora timer cancellation #14422

merged 4 commits into from
Mar 12, 2021

Conversation

pan-
Copy link
Member

@pan- pan- commented Mar 11, 2021 •
edited
Loading

Summary of changes

Fix abusive use of LoRaWANTimer::stop. Events were canceled even when they were being executed or after their execution.
This change reset the event ID to 0 before calling the callback.

A new API has been added to achieve this task: LoRaWANTimer::clear

Documentation

None

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 

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

@Swap-File

@pan-
Lora: Add clear method to LoRaWANTimer.
4885739 
This function must be called by the callback registered in init.
It clears the timer_id to prevent abusive use of the stop methods on expired timer.
@pan-
Lora: Clear callback ID when timer callback is executed.
9781418
@pan- pan- changed the title Lora cancellation Fix Lora timer cancellation Mar 11, 2021
@pan- pan- mentioned this pull request Mar 11, 2021
Merged
@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested review from a team March 11, 2021 21:30
@Swap-File
Copy link

I'll fire up a set of devices for testing and let it run over the weekend. I should be able to spare at least 10 devices.

0xc0170
0xc0170 previously approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

I'll edit the file to add a new line to get this into CI now

@mergify mergify bot dismissed 0xc0170’s stale review March 12, 2021 08:24

Pull request has been modified.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 12, 2021

CI started

0xc0170
0xc0170 previously approved these changes Mar 12, 2021
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: review labels Mar 12, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 12, 2021

Please review unittests failures (related to the changes)

@mbed-ci
Copy link

mbed-ci commented Mar 12, 2021

Jenkins CI Test : ❌ FAILED

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-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️

paul-szczepanek-arm
paul-szczepanek-arm previously approved these changes Mar 12, 2021
View reviewed changes
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.

I would class this as an bugfix - not a feature update.

@mergify mergify bot dismissed stale reviews from 0xc0170 and paul-szczepanek-arm March 12, 2021 09:49

Pull request has been modified.

@pan-
Copy link
Member Author

pan- commented Mar 12, 2021

@0xc0170 Fixed, I change the PR qualification as bug fix: The API added is internal to the stack.

@mbed-ci
Copy link

mbed-ci commented Mar 12, 2021

Jenkins CI Test : ✔️ SUCCESS

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

CLICK for Detailed Summary

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

@0xc0170 0xc0170 merged commit b4abfe6 into ARMmbed:master Mar 12, 2021
@mergify mergify bot removed the ready for merge label Mar 12, 2021
This was referenced Mar 14, 2021
Closed
Closed
@Swap-File Swap-File mentioned this pull request Mar 15, 2021
Closed
This was referenced Mar 16, 2021
Closed
Closed
@Eric-A-Marks Eric-A-Marks mentioned this pull request Mar 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@paul-szczepanek-arm paul-szczepanek-arm paul-szczepanek-arm left review comments

Assignees
No one assigned
Labels
component: connectivity release-version: 6.9.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pan- @ciarmcom @Swap-File @0xc0170 @mbed-ci @paul-szczepanek-arm @mbedmain