rossengeorgiev / aprs-python

📡 Python module for working with APRS
http://aprs-python.readthedocs.io/en/latest/
GNU General Public License v2.0
114 stars 37 forks source link

Support for the 'more recent' reply/ack msg format from 1999 #61

Closed joergschultzelutter closed 2 years ago

joergschultzelutter commented 3 years ago

The new reply/ack format from 1999 introduces a different format hello{AB} (AB = msgNo) which the current library version could not parse in a proper way (restriction also applies to ack/rej messages with that new format). Additionally, it introduces a new optional "ackMsgNo" field: hello{AB}CD (AB = msgNo,CD = ackMsgNo)

Note that the format description lacks clarification on the ackMsgNo's valid character set - bots like e.g. WXBOT only seem to use this field in conjunction with alphanumeric content (AA to ZZ). As Bob/WB4APR has released this change as a backwards-compatible modification, I made the assumption that digits are supported, too.

hemna commented 2 years ago

This would be nice to get in. APRSD is heavily using aprslib now I have a new APRS service called 'REPEAT' using aprsd and aprs-python. :)

http://aprs-repeat.hemna.com https://www.youtube.com/watch?v=3kcmkaLcKNk

joergschultzelutter commented 2 years ago

I have a new APRS service called 'REPEAT' using aprsd and aprs-python. :)

Thank you @hemna for your feedback and your encouragement. My bot is also based on aprslib and faces the same issue. Coincidentally, it even supports a similar "repeater" feature 😃 mainly targeting EU users with full support for both imperial and metric distance units (based on the user's call sign origin).

I bypassed that aprslib limitation by post-processing the aprslib response as part of my own software. My preferred option would be that the 'more recent' ack/rej msg format is supported by aprslib on a native level - which was the reason for me to file this PR. The PR's content is partially based on my bot's older code. As far as I can tell, both 'old' and 'new' (well, 1999 is not really recent , though ...) formats should work with my PR code changes.

hemna commented 2 years ago

I have a new APRS service called 'REPEAT' using aprsd and aprs-python. :)

Thank you @hemna for your feedback and your encouragement. My bot is also based on aprslib and faces the same issue. Coincidentally, it even supports a similar "repeater" feature 😃 mainly targeting EU users with full support for both imperial and metric distance units (based on the user's call sign origin). Very nice. Mine does the same. Have a go at it. send an aprs message 'n 1' to 'REPEAT'

I bypassed that aprslib limitation by post-processing the aprslib response as part of my own software. My preferred option would be that the 'more recent' ack/rej msg format is supported by aprslib on a native level - which was the reason for me to file this PR. The PR's content is partially based on my bot's older code. As far as I can tell, both 'old' and 'new' (well, 1999 is not really recent , though ...) formats should work with my PR code changes.

joergschultzelutter commented 2 years ago

@rossengeorgiev - kindly provide an update on this PR's status - are you ok with the proposed modification?

For both my bot as well as my Robot Framework RPA library, I rather solely rely on your library's parser than applying some post-processing steps to each of my software modules. As the 'more recent' format is an official one, we should ensure that your library can parse those message formats, too.

Thanks!

hemna commented 2 years ago

My users are getting hit with invalid requests due to not being able to parse this format correctly. Can we get this one approved please?

hemna commented 2 years ago

Is this project dead?

joergschultzelutter commented 2 years ago

That would be my guess, too. Haven't seen any updates recently.

rossengeorgiev commented 2 years ago

Hi everyone, I've not really looked at this project for a long time. I see the are some PR to add functionality and fix issues. This one seems pretty complete, but I haven't looked into in detail. I'll try to spend some time tonight to review what's been happening.

joergschultzelutter commented 2 years ago

Thanks for the update - feel free to get in touch if you have any questions.

rossengeorgiev commented 2 years ago

Ok, here are my observations.

  1. The include test file is not using unittest to define the test. Doesn't follow how other tests are strucutred
  2. The parsing code conditionals can use clean up, and there is repeating code. Can be hard to follow
  3. No message examples of the new rej/ack format. This avoids having to hunt around the spec docs
  4. Not sure why github actions is not running the test checks
ldeffenb commented 5 months ago

Just a very late observation here. This fix is actually over-complicated. If the message "sequence" ID was simply treated as 1 to 5 printable characters, then nothing more would need to be done. The } in the middle would just go along for the ride and unless you're actually implementing ReplyAcks, you have no need to know that there's 2 parts delimited by that }.