plewin / tp-link-modem-router

Goodies for TP-Link modem routers
GNU General Public License v3.0
82 stars 17 forks source link

SMS sent but with error #6

Closed simonporte closed 3 years ago

simonporte commented 3 years ago

Hello,

‪TL-MR6400 v5 firmware:‪1.1.0 0.9.1 v0001.0 Build 200511 Node 14.16.0 on Windows

I send SMS, it works but returns an error. Have you any clue why ?

image

Thanks.

plewin commented 3 years ago

Hi, I believe your router is slow to process the SMS sending

When a SMS is send via this tool, the tool asks for a result immediately but interpret a "queued" state as failed. The code could be improved by waiting a bit more and retrying until a timeout before declaring the sending as failed

simonporte commented 3 years ago

Thanks for the quick answer. When I first tested, it took something like 5-8 sec to complete the command. I retried and speed was much better, SMS recevied after 2/3 seconds. I don't know how many time the script waits but 5 seconds seems safe, perhaps even more.

The slowness is strange, the router is connected by cable with wifi and DHCP disabled, there should be no stress on it.

I notice sending SMS disconnects from the web UI. Do you know if it is possible to send SMS from several machines at the same time ?

plewin commented 3 years ago

I retried and speed was much better,

If a session is still open on the router ui the tool fails its first connection.

The slowness is strange, the router is connected by cable with wifi and DHCP disabled, there should be no stress on it.

Either the gsm connection is less than ideal or the sending is scheduled differently than the mr600 i used to write the code. I don't know the specifics of the scheduling so I can't provide data about how long one should expect the router to send the sms.

As for the waiting, i will not implement a waiting time. I will however rewrite the error as a warning to avoid confusion. Later if I find time for that i will implement an automatic retry until the sending is marked as processed by the router.

I notice sending SMS disconnects from the web UI.

It is a known documented limitation

Do you know if it is possible to send SMS from several machines at the same time ?

Thing is, the router does not really offer an api and this code simulate an admin user sending a sms using the web ui. The router disconnect every sessions at each login and IFAIK there is no way to add users as a workaround. It should be safe to use the command tool by multiple computers if the sending frequency is low, each computer would automatically disconnect the others, I would say that the code is quite reliable. It's possible that a sending fails if a second computer initiates a connection while the first compute just finished connecting and start sending the sms. If you really need high frequency and reliability, queuing could be implemented in the api bridge or you could make your own queue to call the command line tool once per message and only once at a time

Edit: Note that the api bridge is more reliable at sending sms than multiple computers with the cli tool because there is only one connection to maintain with the router when the api bridge is used. There are still risks if someone connects to the web api and if there are multiple sms at the exact same time and a reconnection was necessary but these risks are limited

simonporte commented 3 years ago

If a session is still open on the router ui the tool fails its first connection.

When the error triggered, I did a second attempt which was identical with error return ; The web UI was disconnected by the first try, and I did not reconnect It as far as I remember.

Note that the api bridge is more reliable at sending sms than multiple computers with the cli tool

Thank you for pointing this out. It is exactly what I needed. I managed to send SMS in Home Assistant and Checkmk, both with a curl command. I did not try both at the same time but it seemed pretty reliable.

If you really need high frequency and reliability, queuing could be implemented in the api bridge

There is no queuing in the bridge ?

Just for your information, I encountered an error in Home Assistant. The space in -H "accept: application/json" does trigger an error because of YAML parsing. The space can be removed safely, maybe it can be removed from the readme?

Full line:

# Sending SMS application/x-www-form-urlencoded style
curl --user apiuser:pleasechangeme -d to=0123456789 -d content=test1 -X POST "http://127.0.0.1:3000/api/v1/sms/outbox" -H  "accept: application/json"

I notice you marked docker as a planned feature. Do you plan to do it soon? If not I plan to do it for the api brige if I have the time, may I share mine and do a pull request? I will do nothing fancy but perhaps it would help. In this case, do you prefer Alpine of Debian as a base image?

plewin commented 3 years ago

There is no queuing in the bridge ?

There is no queuing implemented in the api bridge because it's not worth it imo Even with 2 curls at the same time it will be fine unless the api bridge needs to (re)connect because the communication from the api bridge to the router is somehow atomic unless there's a reconnection involved. There are some race condition paths that would allow a sending to fail but there are pretty hard to reach. You need to connect manually to the ui at the exact same time of a sms sending to reach these paths so that's rare. Any use case that would require better reliability than what's already implemented should not rely in this router anyway imho.

The space can be removed safely, maybe it can be removed from the readme?

Which space exactly ? The one in "accept: xx" ? It's supposed to be here. I believe that's not an issue in this project but an escape issue in the yaml that used the command. Anyway -H "accept: application/json" is not actually necessary and have no effects, it's to make clearer in the doc that the result body will be json

I notice you marked docker as a planned feature. Do you plan to do it soon?

It's planned but I can offer no release date. Maybe soon because I know now that there's interest. I wouldn't release it until I am confident about its quality. It will be based on official node alpine.

You can share yours if you make one and I would review it, and link to it in the readme if you make one first and you are willing to provide support. But I would probably write my own anyway.

plewin commented 3 years ago

Hi @simonporte, do you still need assistance on this issue ? Best regards,

simonporte commented 3 years ago

Hello,

As for the issue, you can mark it as closed.

I confirm the api bridge suited my needs and is pretty reliable.

It is running 24x7 for almost two month, sending something like 30 SMS per day approximately, sometimes a lot more. They are often grouped, 3/4 SMS at the same time with no problem whatsoever.

As discussed before, I share you my docker config below.

You need to put a valid config.json in the same folder as the dockerfile

The dockerfile is two-stage build. I think it can be easily adapted for use with the gateway. The docker-compose.yml is for use with docker-compose.

dockerfile

FROM node:lts-alpine
WORKDIR /home/node
RUN apk --no-cache add curl
RUN curl -s -L -O "https://github.com/plewin/tp-link-modem-router/archive/master.zip"
RUN unzip master.zip
WORKDIR /home/node/tp-link-modem-router-master
run yarn install

FROM node:lts-alpine
WORKDIR /home/node/tp-link-modem-router-master
COPY --from=0 /home/node/tp-link-modem-router-master .
CMD ["node", "./api-bridge.js"]

docker-compose.yml

version: "2.2"
services:
  tp-link-api-bridge:
    build: .
    container_name: tp-link-api-bridge
    volumes:
      - ./config.json:/home/node/tp-link-modem-router-master/config.json
    ports: 
      - 3000:3000
    restart: unless-stopped
    # If you use watchtower, uncomment these lines to disable autoupdate
    #labels:
    #  com.centurylinklabs.watchtower.enable: "false"
    init: true
plewin commented 3 years ago

Thanks a lot for the feedback! I appreciate it.

Your Dockerfile and docker-compose.yml looks quite good to me. Would you mind contributing these two files to this repository with a pull request ? Preferably without the watchtower comment in docker-compose.yml

I will document the readme and make smalls adjustments in the Dockerfile to not hardcode the master ref

simonporte commented 3 years ago

Hello, I did the PR. Regards.

plewin commented 3 years ago

I merged it. Thanks a lot !