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

SLh and SLg interfaces for LTE location services. #111

Merged
merged 20 commits into from
Sep 13, 2017
Merged

Conversation

FerUy
Copy link
Contributor

@FerUy FerUy commented Jul 1, 2017

This is a pull request for issues #37 and #38, concerning AVP definition and configuration for LTE location services.

As agreed, this one replaces old PR #48. All comments/traces/logs there apply here as well, but opposed to the latter, this work includes only one commit with fixes needed as per latest changes in Restcomm jdiameter project during the last months. All testsuite tests are passing. Likewise, maven project is building successfully with no checkstyle errors.

Copy link
Contributor

@ammendonca ammendonca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @FerUy,

Great contribution for SLg and SLh interfaces. Here are a few comments:

  • Regarding added files, in some of them you are using the dual license header, while only the TeleStax license header should be used;
  • In org/jdiameter/api/slg/ClientSLgSession.java you are defining both sendProvideLocationRequest and sendLocationReportRequest (and the inverse for Server, regarding Answers). It is not the same entity sending both requests (GMLC sends PLR and LRA, receives PLA and LRR, the reverse for MME/SGSN), so it's supposed that Client has the methods to send the PLR and LRA and the Server has the methods to send the LRR and PLA. The same logic should apply to listeners. This also has impact on the FSM event handling.
  • On the *SessionImpl (eg: here) the setState method call with the TERMINATED state already calls the cancelMsgTimer method, still you are calling it again. That second call may be removed
  • We don't need to define the methods for checking existence and getting the value for all the AVPs which the message supports, as you did (eg in org/jdiameter/api/slg/events/LocationReportRequest.java), we only define for those that are used internally for FSM or other internal puproses;
  • In dictionary.xml:
    • The Destination-Host AVP is not mandatory in the RIR, as you've made it here. We usually also try to define Destination-Host and Destination-Realm as forbidden in answers as in here, as this is a global Diameter rule and a typical cause to problems. Feel free to do so as well.
    • The message definitions are including non top-level AVPs, those should not be part of the message definition (eg, in PLR the LCS-Name-String, LCS-Format-Indicator, etc.. in RIA the SGSN-Number/Name/Realm, etc...). Please fix accordingly.

I will still go through the testsuite logic, overall it seems OK but I want to understand better the diverse kind of tests included (Feel free to clarify).

Thanks for the great effort!

@ammendonca ammendonca removed the request for review from brainslog July 10, 2017 17:36
… the TeleStax one -updated ad per latest version of RnD Open Source Playbook-. Issues #37 and #38, PR #111
@FerUy
Copy link
Contributor Author

FerUy commented Jul 11, 2017

Thanks @ammendonca!

Just pushed commit 88c661c where I only fixed the dual license problem (and only in the SLh or SLg new files, didn't touch the other ones modified for this PR but which were already present). As this was a minor change involving only comments, I thought it would be better for you to have those changes in only one commit and not mixed with the other change requests.

I couldn't work as much as I would have liked today in this matter due to other tasks, hopefully I'll go over the other change requests in the next 24 hours. Thanks again!

Fernando Mendioroz added 2 commits July 11, 2017 23:08
…ter setState has already achieved it when session state is TERMINATED. Issues #37 and #38, PR #111
… RIR and setting Destination-Host and Destination-Realm as forbidden in RIA, PLA and LRA. Issues #37 and #38, PR #111
Fernando Mendioroz added 11 commits July 24, 2017 01:10
…RA and LRA methods and derived changes. Issues #37 and #38, PR #111
… unique class names for avoiding confusion with equally named ones for other interfaces. Issues #37 and #38, PR #111
…n IDLE case, which made remaining SLg test pass for deferred location services. Issues #37 and #38, PR #111
… methods included client/server ones -all tests passing-, explaining immediate and deferred LCS referencing 3GPP specs, plus some esthetic enhancements, also in SLh testsuite. Issues #37 and #38, PR #111
@FerUy
Copy link
Contributor Author

FerUy commented Aug 21, 2017

Hi @ammendonca, I've completed almost all of your change requests, including the most tricky one, with all tests passing. Not including all AVPs is the only change request pending, which I will do in next commit to finish this. Just for the record, I'm including here traces of test suite including all requests/answers for SLh and SLg including all AVPs (likewise, I include Wireshark Diameter dictionary.xml including the AVPs that are still not included there as for version 2.2.7 for Ubuntu... if you apply it, Wireshark will recognize all AVPs nicely).

In commit 1cbf096, I included separated classes for only managing each Diameter message for SLg (PLR, PLA, LRR and LRA), but left the previous ones for testing a complete immediate-deferred location service basic exchange back and forth between GMLC and MME. So, you have 3 tests for SLg, one only including PLR/PLA (GMLC as client, MME as server), another one only including LRR/LRA (opposite roles as previous for GMLC and MME), and a final one including all of them, PLR/PLA followed by LRR/LRA. Also included extensive comments in the testsute for complete understanding of both interfaces, messages exchanged, goals and roles.

So, almost there... if nothing strange happens, I will commit the last requested change tomorrow.

SLh-SLg_traces_all_AVPs.zip

Fernando Mendioroz added 2 commits August 22, 2017 20:53
…us fixed LCS-Supported-GAD-Shapes AVP definition and added new specified grouped AVPs Motion-Event-Info and Additional-Area. Issues #37 and #38, PR #111
…ue for AVPs in SLg -change requested-, added new SLg AVP codes and removed duplicated Motion-Event-Info AVP definition in dictionary. Issues #37 and #38, PR #111
@FerUy
Copy link
Contributor Author

FerUy commented Aug 23, 2017

@ammendonca I believe I have completed everyone of your change requests.
All yours :)

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

Successfully merging this pull request may close these issues.

3 participants