linxGnu / gosmpp

Smpp (3.4) Client Library for Go
Apache License 2.0
152 stars 59 forks source link

[GSM7BITPACKED] Fixes around Split/ShouldSplit #136

Closed krstdmr closed 3 months ago

krstdmr commented 7 months ago
  1. GSM7BITPACKED - Changes about adding a padding bit to the beginning of the septets just after UDH during EncodeSplit Ref1: https://www.etsi.org/deliver/etsi_ts/123000_123099/123040/16.00.00_60/ts_123040v160000p.pdf Page 74 Ref2: https://help.goacoustic.com/hc/en-us/articles/360043843154--How-character-encoding-affects-SMS-message-length Pls. ref. to the note "..It is added as padding so that the actual 7-bit encoding data begins on a septet boundary—the 50th bit." Ref3: https://en.wikipedia.org/wiki/Concatenated_SMS "..This means up to 6 bits of zeros need to be inserted at the start of the [message]."
  1. Handling escape chars during Split/ShouldSplit Esacpe characters occupy 2 octets/septets Ref1: https://en.wikipedia.org/wiki/GSM_03.38 Ref2: https://www.developershome.com/sms/gsmAlphabet.asp

  2. Fix for UCS2-> ShouldSplit where the first segment should be split just after 70 UCS2 chars (rune check, pls see unit tests), but the next after 67 chars.

  3. Fix for EsmClass when Split returns a single part

krstdmr commented 7 months ago

PR review from any of you is appreciated. @linxGnu || @goten4 || @laduchesneau. Thanks in advance.

laduchesneau commented 7 months ago

Ive reviewed the changes, all looks good. I will also run test on my end.

To be honest, packed GSM never worked for me with gosmpp, even thou my SMSC supports it. So ill test these changes today and get back to you.

krstdmr commented 7 months ago

@laduchesneau Thanks. I have just pushed one minor fix more. Please see the ref. below; Ref : https://en.wikipedia.org/wiki/GSM_03.38 "When there are 1 to 6 spare bits in the last octet of a message, these bits are set to zero (these bits do not count as a character but only as a filler). When there are 7 spare bits in the last octet of a message, these bits are set to the 7-bit code of the CR control (also used as a padding filler) instead of being set to zero (where they would be confused with the 7-bit code of an '@' character)."

laduchesneau commented 7 months ago

LGTM (mostly)

I opened a comment/question for the code block added in Shortmessage.go

krstdmr commented 7 months ago

Hi @laduchesneau. Have you had chance to test your SMSC? We had to active 7-bit coding in SMSC before using it.

laduchesneau commented 7 months ago

Hi, I did test and it did not work....but its not the library, its the SMSC, its currently configured for 8bit GSM.

That said, I did test your code with our old internal tool using cloudhopper-commons and the bytes match.

I cant test the split function, we don't use it, but the encoders work.

krstdmr commented 7 months ago

Yeah, makes sense. As I said, that was something configurable in our SMSC, worked after we activated. Great that you verified with cloudhopper-commons.

linxGnu commented 6 months ago

@laduchesneau I don't have the SMSC here to test 7bitpacked too. Since it work for @krstdmr with his SMSC, I think its good enough to merge this PR. HDYT?

linxGnu commented 6 months ago

@krstdmr

the PR also breaks gsm7bit none-packed. Could you please revert the change, only update packed part.

Something like this:

In this way, it's easy to review + keep working version of gsm7bit non-packed

krstdmr commented 6 months ago

Hi @linxGnu . Sure, I can try to change as you suggested in the beginning of the next week. But could you please point the place where it breaks, or any hints about what happens so that it might be easier for me to understand the problem?

krstdmr commented 5 months ago

@linxGnu Hi again. I have pushed the changes as you requested. (Introduced gsm7bitPacked type instead). I appreciate if any of you review the PR when you have time.

linxGnu commented 3 months ago

It's great to hear that @laduchesneau

For SMPP, battle test in PROD is the best trustable source.

linxGnu commented 3 months ago

Thank you for great contribution @krstdmr @laduchesneau

krstdmr commented 3 months ago

Great to see the changes er merged. Thanks for a nice collaboration @laduchesneau @linxGnu