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

SMS: Increase timeout for send and get. Fix text SMS parsing when CRLF is contained in the text. #15477

Merged
merged 3 commits into from
Jun 8, 2024
Merged

SMS: Increase timeout for send and get. Fix text SMS parsing when CRLF is contained in the text. #15477

merged 3 commits into from
Jun 8, 2024

Conversation

davidalo
Copy link
Contributor

@davidalo davidalo commented Dec 23, 2023 •
edited
Loading

Summary of changes

  1. For some devices sending can be slow (as an example see SIM800, it can be up to 60s) and we are returning an improper time out. Commit
  2. When SMS list is big and baudrate is not fast enough, we can suffer from timeouts while getting a sms because it is parsing the full list. Commit
  3. When parsing SMS, it can happen that we receive CRLF in the SMS payload (happened to me when receiving provider texts). So, let's say we receive
Hello <CR><LF>
World!

With the current implementation, second consume_to_stop_tag was stopping in <CR><LF> and rest of the code was failing for obvious reasons. With this patch we consume the full payload as bytes.
Commit

Impact of changes

Affects all cellular targets.

Migration actions required

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 

[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

Further tests would be desirable to cover this case.

Reviewers

@0xc0170
Copy link
Contributor

0xc0170 commented Dec 28, 2023

Please split into separate commits unrelated changes (adding new functionality vs bugfixing or similar).

@davidalo
Copy link
Contributor Author

Please split into separate commits unrelated changes (adding new functionality vs bugfixing or similar).

@0xc0170 Done. 3 commits now with the three different changes.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 6, 2024

Thanks, can you add details also to the commit messages please? Why are we increasing the timeout, updating parsing SMS? All should be answered there.

@davidalo
Copy link
Contributor Author

davidalo commented Apr 5, 2024

Thanks, can you add details also to the commit messages please? Why are we increasing the timeout, updating parsing SMS? All should be answered there.

@0xc0170 Done, can you review?

@davidalo davidalo force-pushed the fix-sms branch 2 times, most recently from 516d0e6 to e13a7e1 Compare April 5, 2024 14:44
@davidalo
Increase AT timeout to 10s in AT_CellularSMS::send_sms
5c49f16 
For some devices sending can be slow (as an example see SIM800, it can be up to 60s), command is being run properly but default timeout is returning an invalid error.
See https://www.elecrow.com/wiki/images/2/20/SIM800_Series_AT_Command_Manual_V1.09.pdf
@davidalo
Increase AT timeout to 10s in AT_CellularSMS::get_sms
d676084 
When SMS list is big and baudrate is not fast enough, with default timeout we can suffer from timeout error while getting a sms because method is parsing the full list and this takes long.
0xc0170
0xc0170 previously approved these changes Apr 10, 2024
View reviewed changes
@davidalo
Copy link
Contributor Author

@0xc0170 With all these changes of splitting commits and so on it seems that I messed up at some point. I will review code and ask for approval again.

@mergify mergify bot dismissed 0xc0170’s stale review April 23, 2024 12:59

Pull request has been modified.

@davidalo
Copy link
Contributor Author

@0xc0170 I've just fixed the build, missed variable when amending commits. I also fixed SMS test which was broken due to new list_messages implementation and new ATHandler methods. Only last commit was changed.

Could you please review again?

@davidalo davidalo requested a review from 0xc0170 April 23, 2024 13:29
0xc0170
0xc0170 previously approved these changes Apr 24, 2024
View reviewed changes
@0xc0170 0xc0170 added needs: CI release-type: patch Indentifies a PR as containing just a patch and removed needs: review labels Apr 24, 2024
@0xc0170
Copy link
Contributor

0xc0170 commented May 6, 2024

CI started

@mbed-ci
Copy link

mbed-ci commented May 6, 2024

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_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_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-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ❌

@mergify mergify bot added needs: work and removed needs: CI labels May 6, 2024
@davidalo
Fix AT_CellularSMS::list_messages breaking in text mode when CRLF is …
9e7e22d 
…contained in SMS payload text

When parsing SMS, it can happen that we receive CRLF in the SMS payload (happened to me when receiving provider texts).
As an example, we can receive:

"""
Hello <CR><LF>
World!
"""

With previous implementation, second consume_to_stop_tag was stopping in <CR><LF> and rest of the code was failing for obvious reasons.
With this commit we consume the full payload as bytes.
@mergify mergify bot dismissed 0xc0170’s stale review May 11, 2024 08:43

Pull request has been modified.

@davidalo
Copy link
Contributor Author

Fixed astyle issue. Not sure what's the problem with jenkins-ci/mbed-os-ci_greentea-test, I can not see the logs. Could you please offer more details @0xc0170 ?

@0xc0170
Copy link
Contributor

0xc0170 commented May 27, 2024

CI restarted

I'll check the logs once done. There are some devices that are offline unfortunately (status fail reported)

@mbed-ci
Copy link

mbed-ci commented May 28, 2024

Jenkins CI Test : ❌ FAILED

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

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_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-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ❌

@mergify mergify bot added needs: work and removed needs: CI labels May 28, 2024
@davidalo
Copy link
Contributor Author

davidalo commented Jun 8, 2024

@0xc0170 Please offer more details so I can fix.

@0xc0170 0xc0170 merged commit 20340d7 into ARMmbed:master Jun 8, 2024
18 of 20 checks passed
@mergify mergify bot removed the ready for merge label Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 left review comments

Assignees
No one assigned
Labels
release-type: patch Indentifies a PR as containing just a patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@davidalo @0xc0170 @mbed-ci