Connection-Oriented SCCP: CR & CC, to start with#29
Connection-Oriented SCCP: CR & CC, to start with#29dmisol wants to merge 14 commits intowmnsk:masterfrom
Conversation
wmnsk
left a comment
wmnsk
left a comment
There was a problem hiding this comment.
Great, thanks for your contribution! Before merging, can you please;
- read through inline comments and fix them (including what golangci-lint claims).
- add GoDoc comments over every exported function.
- add test cases to sccp_test.go instead of adding _test files for each.
cc.go
Outdated
| /* | ||
| Message type 2.1 F 1 | ||
| Destination local reference 3.2 F 3 | ||
| Source local reference 3.3 F 3 | ||
| Protocol class 3.6 F 1 | ||
| Credit 3.10 O 3 | ||
| Called party address 3.4 O 4 minimum | ||
| Data 3.16 O 3-130 | ||
| Importance 3.19 O 3 | ||
| End of optional parameter 3.1 O 1 | ||
| */ | ||
| type CC struct { |
There was a problem hiding this comment.
We don't need such a detailed reference to the spec, and please follow the Go's convention of writing a comment as a doc (godoc). You can see udt.go for reference.
| CalledPartyAddress *params.PartyAddress | ||
| } | ||
|
|
||
| func ParseCC(b []byte) (*CC, error) { |
There was a problem hiding this comment.
Please follow the function order (see udt.go and scmg.go). Also, constructor (NewCC) is missing.
| return msg, nil | ||
| } | ||
|
|
||
| func (msg *CC) UnmarshalBinary(b []byte) error { |
There was a problem hiding this comment.
| func (msg *CC) UnmarshalBinary(b []byte) error { | |
| func (c *CC) UnmarshalBinary(b []byte) error { |
Please use a single character c as a receiver, which is a convention of Go.
| func (msg *CC) String() string { | ||
| if msg.CalledPartyAddress != nil { | ||
| return fmt.Sprintf("{Type: CC, CalledPartyAddress: %v}", msg.CalledPartyAddress) | ||
| } | ||
| return "{Type: CC}" | ||
| } |
There was a problem hiding this comment.
Do similar to what UDT and SCMG do.
cc.go
Outdated
| } | ||
|
|
||
| func (msg *CC) MessageTypeName() string { | ||
| return "CR" |
There was a problem hiding this comment.
| return "CR" | |
| return "CC" |
| @@ -0,0 +1,34 @@ | |||
| package params | |||
There was a problem hiding this comment.
I don't think we need a type definition for the local reference. Instead, you can define the source/destination local reference as uint32, and you can cut off the excessive octet (the easiest way is to use these utility functions).
| m = &CR{} | ||
| case MsgTypeCC: | ||
| m = &CC{} | ||
| /* TODO: implement! case CREF: |
There was a problem hiding this comment.
| /* TODO: implement! case CREF: | |
| /* TODO: implement! | |
| case CREF: |
| Opts []*params.Optional | ||
|
|
||
| Data *params.Optional | ||
| CalledPartyAddress *params.PartyAddress |
There was a problem hiding this comment.
Handling of the optional parameters could be nicer, but it's due to the bad-designed parameter handling in the current code base. I will work on refactoring parameters.
There was a problem hiding this comment.
Sorry, let me comment on this
In fact, for the practical case, I just need convenient access to Data and Called/Calling pty-s only.
To my knowledge, the only interface where connection-oriented SCCP is used, is GSM A-interface, and I can hardly imagine the real use of other params there.
I would appreciate to avoid making things complicated
There was a problem hiding this comment.
Q.713 is such a small spec and I'm more comfortable to just have everything implemented rather than to ignore some of them. I think I can quickly make it done soon-ish.
|
ok, thx |
I'd appreciate if you can try to get CC and CR merged first. I think it'd be less work for us both (to avoid reviewing everything at once with the same fixes that may apply to all messages). |
|
I have implemented all the params and XUDT as an example that has optional parameters. Please take a look at the latest master branch and continue your work :) |
2 participants
No description provided.