Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default sequence value #17

Closed
wants to merge 2 commits into from
Closed

Change default sequence value #17

wants to merge 2 commits into from

Conversation

daper
Copy link
Contributor

@daper daper commented Nov 1, 2014

0 is out of range: 0x00000001 to 0x7FFFFFFF (1 to 32767)

0 is out of range: 0x00000001 to 0x7FFFFFFF (1 to 32767)
@daper
Copy link
Contributor Author

daper commented Nov 1, 2014

In SMPP, the sequence number may range from 0x00000001 to 0x7FFFFFFF (1 to 32767) - it is equivalent to a positive signed 16 bits integer.

The sequence number should be increased monotonically for each submitted SMPP request and in case it reaches its maximum value, it should be reset to 0x00000001.

For more details you can take a look to SMPP v3.4 Specification - chapter 5.1.4 and SMPP v5.0 Specification - chapter 4.7.24.

Referrer: smpp-session-sequence-id-max-value

@farhadi
Copy link
Owner

farhadi commented Nov 1, 2014

Thanks for this PR, but Session.sequence keeps the last sequence_number that have been sent over the session and when you send a pdu, its value is increased before being assigned to pdu.sequence_number :
https://github.com/farhadi/node-smpp/blob/master/lib/smpp.js#L67

But about resetting it to 1 in case of reaching the maximum value, if you fix it and update the PR, I'm willing to merge it.

@daper
Copy link
Contributor Author

daper commented Nov 1, 2014

I have been experimented an error and I solved it with this change. But you are right... Let me check it, thanks. I'll update PR.

Also sets the default in 0. (undo)
@daper
Copy link
Contributor Author

daper commented Nov 7, 2014

I could not reproduce the error :( So I have only added the reset line. Thank kyou farhadi.

@farhadi
Copy link
Owner

farhadi commented Feb 4, 2015

I've merged your commit with a little modification in 268b055.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants