gosmpp icon indicating copy to clipboard operation
gosmpp copied to clipboard

GSM7BITPACKED related changes and some other fixes around Split/ShouldSplit

Open krstdmr opened this issue 1 year ago • 11 comments

  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]."

  2. 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

  3. 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.

  4. Fix for EsmClass when Split returns a single part

krstdmr avatar Feb 07 '24 11:02 krstdmr

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

krstdmr avatar Feb 07 '24 11:02 krstdmr

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.

laduchesneau avatar Feb 07 '24 13:02 laduchesneau

@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)."

krstdmr avatar Feb 07 '24 15:02 krstdmr

LGTM (mostly)

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

laduchesneau avatar Feb 08 '24 02:02 laduchesneau

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

krstdmr avatar Feb 13 '24 10:02 krstdmr

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.

laduchesneau avatar Feb 17 '24 15:02 laduchesneau

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

krstdmr avatar Feb 19 '24 08:02 krstdmr

@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 avatar Mar 21 '24 00:03 linxGnu

@krstdmr

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

Something like this:

  • Create a new type coding instead of using just *gsm7bit
  • We can remove gsm7bit's packed field in another PR

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

linxGnu avatar Mar 21 '24 00:03 linxGnu

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 avatar Mar 21 '24 10:03 krstdmr

@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.

krstdmr avatar Mar 25 '24 08:03 krstdmr

It's great to hear that @laduchesneau

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

linxGnu avatar Jun 10 '24 08:06 linxGnu

Thank you for great contribution @krstdmr @laduchesneau

linxGnu avatar Jun 10 '24 08:06 linxGnu

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

krstdmr avatar Jun 11 '24 07:06 krstdmr