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 supportedCommands property for Teleofis modems #62

Merged
merged 2 commits into from
Mar 11, 2019
Merged

Fix supportedCommands property for Teleofis modems #62

merged 2 commits into from
Mar 11, 2019

Conversation

ffix
Copy link

@ffix ffix commented Jul 12, 2018 •
edited
Loading

Some modems like Siemens Teleofis have different AT+CLAC command response:

ATA                                                                      
ATD                                                                      
ATH                                                                      
ATO                                                                      
ATE                                                                      
ATI
...
AT+CECALL
AT+CCED
AT+CSVM

OK

So we need to remove AT prefix to return valid supportedCommands value.

@coveralls
Copy link

coveralls commented Jul 12, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 89.947% when pulling 4abcc93 on ffix:siemens-commands-fix into 0d04314 on babca:master.

tomchy
tomchy reviewed Nov 6, 2018
View reviewed changes
test/test_modem.py
@@ -207,6 +207,8 @@ def writeCallbackFunc(data):
(['FGH,RTY,UIO\r\n', 'OK\r\n'], ['FGH', 'RTY', 'UIO']), # nasty, but possible
# ZTE-like response: do not start with +CLAC, and use multiple lines
(['A\r\n', 'BCD\r\n', 'EFGH\r\n', 'OK\r\n'], ['A', 'BCD', 'EFGH']),
# Siemens response: like ZTE but each command has AT prefix
(['AT&F\r\n', 'AT&V\r\n', 'AT&W\r\n', 'AT+CACM\r\n', 'OK\r\n'], ['&F', '&V', '&W', '+CACM']),
Copy link

Choose a reason for hiding this comment

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

Does Siemens modem actually send & characters?

Copy link
Owner

Choose a reason for hiding this comment

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

AT& commands exist.

AT&F = restore factory defaults.

all & commands here

Copy link
Owner

Choose a reason for hiding this comment

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

Oh you probably meant '&F', '&V', '&W' part. Let's wait for @ffix

Copy link

Choose a reason for hiding this comment

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

Thank you for a reminder! I just got suspicious, because @ffix have not mentioned those commands inside PR description. Sorry for the confusion.

Copy link
Author

@ffix ffix Nov 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does Siemens modem actually send & characters?

It's my mistake. It is actually Teleofis modem, not Siemens.
I've updated description.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for a reminder! I just got suspicious, because @ffix have not mentioned those commands inside PR description. Sorry for the confusion.

You're right. I copied these commands from modem response directly.

@babca
Copy link
Owner

babca commented Nov 6, 2018 •
edited
Loading

Is this kind of data manipulation safe? Couldn't this affect non-Siemens modems as well? I'm not sure if it's a generic solution, ready to merge.

@ffix ffix changed the title Fix supportedCommands property for Siemens modems Fix supportedCommands property for Teleofis modems Nov 30, 2018
@ffix
Copy link
Author

ffix commented Nov 30, 2018

Is this kind of data manipulation safe? Couldn't this affect non-Siemens modems as well? I'm not sure if it's a generic solution, ready to merge.

Unfortunately, I don't have an answer to this question. Now I have only Siemens and Teleofis modems.

@tomchy
Copy link

tomchy commented Mar 11, 2019

Well - in order to properly answer this question, there should be a list of hardware that this library is aiming for. @babca do you have such list on your mind?

Overall - this PR modifies just a single command (+CLAC) and I cannot see how can it break other modems - the AT prefix inside command list is definitely something unexpected.

@babca babca merged commit f446d26 into babca:master Mar 11, 2019
@babca
Copy link
Owner

babca commented Mar 11, 2019

Thanks @tomchy for the second (your) opinion. Merged.

"there should be a list of hardware that this library is aiming for"
Well, we can make an issue thread with a question like "What modem do you successfully use with this library?". Than we can add a statement to README.md like "the library is trying to support all GSM modem brands and models. However, only these modems are currently tested by its users: . Feel free to update this list with your modem."

@tomchy
Copy link

tomchy commented Mar 11, 2019

Sounds like a great idea!
It may not be easy to achieve it, but give an opportunity to get a nice feedback from users.

The list most probably will evolve into a table with tested features as a second dimension/a separate doc page :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@babca babca babca left review comments

@tomchy tomchy tomchy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ffix @coveralls @babca @tomchy