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

drivers/at: add function to read raw data bytes from modem #10191

Merged
merged 1 commit into from
Dec 28, 2018
Merged

drivers/at: add function to read raw data bytes from modem #10191

merged 1 commit into from
Dec 28, 2018

Conversation

fedepell
Copy link
Contributor

Contribution description

This PR adds a function to read raw data bytes from the modem device. All previous functions work on a line basis and are not suitable for some operations like raw data bytes transfer.
My use case for example is transfering via GPRS an image file to flash remotely the device. To do this I start the firmware image transfer by first issuing AT commands and then having to read the raw data bytes and flashing on the device (following concepts from #9342 ).

Testing procedure

The manual test under tests/drivers_at was augmented to call also this function. With a device with a modem connected use on the test shell for example:
send_recv_bytes AT+CGMM 31
The function will send the AT+CGMM and then try to read 31 bytes. If less are found it will wait for the timeout to expire, otherwise return immediately with the 31 bytes.

@miri64 miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 18, 2018
@fedepell
Copy link
Contributor Author

I'm using more this function now as dealing with binary data transfers over modem (ie. AT#SRECV commands to transfer via sockets). Could it be possible to review and see if this could be suitable for merging into master? @kaspar030 maybe?

Thanks a lot!

kaspar030
kaspar030 reviewed Nov 30, 2018
View reviewed changes
drivers/at/at.c Outdated Show resolved Hide resolved
kaspar030
kaspar030 requested changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

almost good to go!

drivers/include/at.h Outdated Show resolved Hide resolved
tests/driver_at/main.c Show resolved Hide resolved
@fedepell
Copy link
Contributor Author

@kaspar030 : many thanks for reviewing! Hope it's in better shape now, I can in case squash when looks fine!

@kaspar030
Copy link
Contributor

@fedepell I'm sorry, now that I see the change I understand that any result < len implicitly means the call ended because of a timeout. That's also why the return documentation said "eventually". Now I think that was totally alright. :) If you agree, please undo that change again. But keep the dropped memset.

You can directly squash, otherwise this is good to go!

@fedepell
Copy link
Contributor Author

@kaspar030 : thanks for the inputs! Ok I reverted the ETIMEDOUT part and left out the memset and squashed all together. I just tried to make the "eventually" comment a bit clear :)

Thanks again for the review!

@kaspar030 kaspar030 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 30, 2018
@fedepell
Copy link
Contributor Author

@kaspar030 : I put as "resolved" the changes some time ago but I believe (sorry if I'm wrong :) ) that this should be somehow confirmed by you, correct? Thanks!

kaspar030
kaspar030 approved these changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK for the change, but I couldn't test it.

@kaspar030 kaspar030 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Dec 18, 2018
@kaspar030
Copy link
Contributor

Could someone give this a quick spin?

@fedepell
Copy link
Contributor Author

@aabadie : could we have this in 2019.01 if we find someone that can try it a little out? thanks!

@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
@aabadie
Copy link
Contributor

aabadie commented Dec 19, 2018

It's added to 2019.01. Since there's already an (Untested) ACK, it is very close from being merged. I won't have the time and am not sure how to test this.

But maybe @leandrolanzieri could help ?

@leandrolanzieri
Copy link
Contributor

Nice. Yes! I can try it out with the R410M modem

leandrolanzieri
leandrolanzieri approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Tested with the ublox r410m modem and works as expected. ACK!

@leandrolanzieri leandrolanzieri added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 19, 2018
@fedepell
Copy link
Contributor Author

Thanks @leandrolanzieri for testing!

For the record: I use it with a Telit GL865-DUAL V3 from a saml21 based hardware.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 27, 2018
@miri64 miri64 merged commit e13bc42 into RIOT-OS:master Dec 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@kaspar030 kaspar030 kaspar030 approved these changes

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
5 participants
@fedepell @kaspar030 @aabadie @leandrolanzieri @miri64