Skip to content

Add features: IMEI, RSSI, Versions, Geolocation, Time, Ring Alerts, handle non-responsive modem #15

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 14 commits into from
Jan 12, 2021

Conversation

zunkworks
Copy link
Contributor

I added functionality to return the modem's IMEI (serial number), RSSI (signal strength), firmware versions, it's geolocation per the Iridium network, the date/time per Iridium network, the ability to enable/disable ring alerts, and lastly the ability to check for ring alerts.

I made changes to handle a non-responsive modem (in case it is powered off) by modifying _uart_xfer to return None if there is no response from the modem. This will require additional changes to several other functions. I'm not sure this was the best way to do it, so I have not updated all the other functions yet. I would like to get some feedback first. If you have a better suggestion, or think this is ok, please let me know and I'll update accordingly.

Thank you,
Jeremy

@zunkworks
Copy link
Contributor Author

Phew, now passes CI checks :)

@ladyada ladyada requested a review from caternuson January 8, 2021 04:11
@caternuson
Copy link
Contributor

caternuson commented Jan 8, 2021

Thanks for these additional updates. I've added a lot of commentary, but please realize they are mostly cosmetic with respect to all the various new properties you've added.

Was there a need for the changes to _uart_xfer? That's the main shared comm function used for the library, so changes to that are a little more scrutinized.
EDIT - Oh, just saw you talk about this a bit in your original post. I'd say revert the _uart_xfer back to the way it was for now. This PR can just focus on the adding of the new properties. And then can open a new issue for dealing with unresponsive modem and discuss there what makes sense.

@zunkworks
Copy link
Contributor Author

Hi Carter, thank you very much for the feedback. I'll start working on the revisions you suggested in your comments. I'll get back to you when I have my code ready for another review or have questions. Thanks again! - Jeremy

@zunkworks zunkworks requested a review from caternuson January 12, 2021 00:21
Copy link
Contributor

@caternuson caternuson left a comment

Choose a reason for hiding this comment

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

Thanks. This is looking good. Added a few more refinements.

Also - have you tested this code with hardware?

… reduced lines of code and memory usage in geolocation and system_time, removed default value in the ring alert setter
@zunkworks
Copy link
Contributor Author

Thanks, I've changed the code per your comments and pushed them.

I have been testing on a 9603 using a Feather M4 Express prior to each commit.
The modem revision I've mostly been testing on:
('Call DSP Version: 1.7 svn: 2358', 'DBB Version: 0x0001 (ASIC)', 'RFA Version: 0x0007 (SRFA2)', 'NVM Version: KVS', 'Hardware Version: BOOT07d2/9603NrevDE/04/RAW0c', 'BOOT Version: TA16005 (rev exported)')
Do you think I should update an example or write a new example to include the new functionality?

Speaking of testing, on the next PR I do I'd like to add +GEMON which returns an estimate of the microamp hours the modem has consumed. I think it would be very useful for anyone running off batteries/solar/etc. However the software revision I have on several modems does not support this. I do have an older modem that does support it, and RockBlock support shows it working on a newer revision than what I have. It could be added as long as it can handle the return code of ERROR. I've written this code, but didn't push it since it wasn't working on all of my modems.

And speaking of errors and future PRs, if the modem is powered down, then the init fails because it calls reset:
Traceback (most recent call last):
File "code.py", line 22, in
File "./adafruit_rockblock.py", line 60, in init
File "./adafruit_rockblock.py", line 80, in reset
File "./adafruit_rockblock.py", line 72, in _uart_xfer
File "./adafruit_rockblock.py", line 70, in
TypeError: 'NoneType' object is not iterable

Which is because line is equal to None, and the while loop can't iterate over None.
while not any(EOM in line for EOM in (b"OK\r\n", b"ERROR\r\n")):
line = self._uart.readline()
resp.append(line)

That's why I was changing reset and _uart_xfer. But we'll save that for another PR as you suggested.

@zunkworks zunkworks requested a review from caternuson January 12, 2021 17:10
@caternuson caternuson merged commit d98b530 into adafruit:master Jan 12, 2021
@zunkworks zunkworks deleted the add-features branch January 12, 2021 20:37
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jan 13, 2021
Updating https://github.com/adafruit/Adafruit_CircuitPython_RockBlock to 1.2.0 from 1.1.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_RockBlock#15 from OperatorFoundation/add-features
  > Added pre-commit-config file

Updating https://github.com/adafruit/Adafruit_CircuitPython_OAuth2 to 1.0.1 from 1.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_OAuth2#1 from brentru/fix-refresh-token
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants