search

CBielstein / APRSsharp

APRS# - Modern APRS software for the amateur radio community
MIT License
12 stars 5 forks source link

Never logs in #161

Open ShawnStoddard opened 3 months ago

ShawnStoddard commented 3 months ago

I built a sample based loosely on the client in the repo. However, it never logged in. I added a \n to the login line in the SendLogin method to resolve. I would love for someone to confirm and if broken, we'll make a change.

CBielstein commented 3 months ago

Thanks for the bug reports! Busy week at the day job, but I'll make some time to investigate soon!

CBielstein commented 2 months ago

@ShawnStoddard Okay, now that day job and "real life" have settled down, I have had time to investigate.

Looks like I wrote a bug in #153 when I changed TcpConnection.cs line 72. This was to change to using raw bytes which works better with TNCs like Direwolf, but when I switched from WriteLine to Write, I lost the automatic line termination logic which was adding newline. The switch was for reason, but I should have added my own logic to add the newline bytes.

I wasn't very thorough with my testing. Automated tests are comparing strings instead of bytes and I must have been excited playing with Direwolf and forgot to verify I hadn't broken APRS-IS functionality. Good catch, thanks for reporting. 😃

Sounds like your comment suggests you'd be willing to make a PR, which I would be receptive of and much more responsive these days.

I think the right change here is something along the lines of:

  1. Add something to send newline after sending bytes in TcpConnection.SendBytes. Something like writer.BaseStream.Write(Encoding.ASCII.GetBytes("\r\n")); (or something more elegant, perhaps).
  2. Maybe we can remove the NewLine set in the constructor? Or maybe it should stay...not sure it hurts or helps.
  3. Consider an update of the tests? Maybe the ReceiveUnitTests.ReceiveHandlesLogin can be updated? Though it's mocking the TcpConnection, so maybe not possible here. Would have to think about how to guard against such a regression in the future...

Like I said, I'd be happy to receive a PR if you're looking to contribute. Else, let me know and I can make some time in the future to adjust this myself.