[GSM7BITPACKED] Fixes around Split/ShouldSplit

[krstdmr]

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]

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

[laduchesneau]

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]

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

Wouldn't this added code block be better placed inside the Encode func for GSM in codings.go ?

I'm just asking.....

[krstdmr]

Thanks for the review @laduchesneau. Actually I tried to move the logic there into the (c *gsm7bit) Encode(str string) method. But the problem is that, this tail cleaning operation is dependent on multi-part / single part type of a message. If Encode() is triggered from a single message then calculation is based on (nSeptet+1) % 8 == 0, because we do not shift the bits left when UDH is not present in a message stream, meaning for a single part message. If Encode() is triggered from a multi-part message (pls see EncodeSplit->shiftBitsLeftOne), then the calculation needs to be based on nSeptet%8 == 0 . Since Encode() is a method that implements the EncDec interface like others, we can not parameterize it. For this reason, I though it can be placed in the SetMessageWithEncoding method. I am not super happy with the placement, but couldn't find any other alternative :) What do you think? Any other place suggestion to move or violate Encode() interface implementation or leave it as it is?

[laduchesneau]

LGTM (mostly)

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

[krstdmr]

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

[laduchesneau]

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]

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]

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

@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

[krstdmr]

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]

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

[laduchesneau]

I did a full review of this PR. This PR has fixes for two issue I recently had:

• When calling ShouldSplit(), if the message is a single segment, the old code would set the UDH bit in EsmClass even thou there is a single segment with no UDH. The fix proposes in this PR is exactly what I implemented in my production code.

• This PR allows me to maximize the number of character I can send in latin-1. I convert the payload into 7bit packed and call should split. Than I decode each segment back into utf-8 and than encode into latin-1. This allow me to send towards SMSCs with DCS3/8 only binds. SMSC will convert the payload into GSM with extended characters, all while the payload is 140 bytes and wont be rejected for MSG_LENGTH_INVALID.

I do believe it bring value to the library.

[linxGnu]

It's great to hear that @laduchesneau

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

[linxGnu]

Thank you for great contribution @krstdmr @laduchesneau

[krstdmr]

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