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

Make TCP use new transport layers #1108

Merged
merged 5 commits into from
Aug 27, 2019
Merged

Conversation

joeygrover
Copy link
Member

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

  • Connect with Manticore and observe code path through SdlSession2 logic.
  • Test multiple transports to ensure TCP transport modifications work in router service

Summary

  • Added a new TCPTransportManager
  • SdlProtocol can now be created with multiplexing or TCP transport config and will use the correct transport manager.
  • Modified TCPTransport to actually use a different thread to write on

Changelog

Enhancements
  • TCP now follows the newer stack

CLA

@codecov-io
Copy link

codecov-io commented Jul 12, 2019

Codecov Report

Merging #1108 into develop will decrease coverage by 1.23%.
The diff coverage is 2.97%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #1108      +/-   ##
============================================
- Coverage      46.73%   45.5%   -1.24%     
- Complexity      4005    4693     +688     
============================================
  Files            453     470      +17     
  Lines          22141   27304    +5163     
  Branches        2513    3686    +1173     
============================================
+ Hits           10348   12424    +2076     
- Misses         11184   14119    +2935     
- Partials         609     761     +152
Impacted Files Coverage Δ Complexity Δ
...smartdevicelink/transport/TCPTransportManager.java 0% <0%> (ø) 0 <0> (?)
...com/smartdevicelink/SdlConnection/SdlSession2.java 20.9% <0%> (-1%) 9 <2> (-1)
...artdevicelink/transport/utl/WiFiSocketFactory.java 82.5% <0%> (-6.69%) 11 <9> (ø)
...artdevicelink/transport/MultiplexTcpTransport.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...n/java/com/smartdevicelink/proxy/SdlProxyBase.java 9.25% <33.33%> (+0.83%) 41 <0> (+14) ⬆️
...smartdevicelink/proxy/rpc/OnDriverDistraction.java 76% <0%> (-24%) 9% <0%> (+4%)
...com/smartdevicelink/proxy/rpc/ImageResolution.java 75% <0%> (-11.67%) 12% <0%> (+5%)
...ava/com/smartdevicelink/proxy/SdlProxyBuilder.java 43% <0%> (-6.37%) 1% <0%> (ø)
...reen/choiceset/CheckChoiceVROptionalOperation.java 22.98% <0%> (-3.25%) 9% <0%> (+3%)
.../managers/screen/menu/BaseVoiceCommandManager.java 53.1% <0%> (-3.2%) 19% <0%> (+3%)
... and 68 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d19c03...96f5237. Read the comment docs.

this.requestedSecondaryTransports = null;
this.requiresHighBandwidth =false;
this.setTransportManager(new TCPTransportManager(config,transportEventListener));
this.requestedPrimaryTransports = Collections.singletonList(TransportType.TCP);
Copy link
Contributor

Choose a reason for hiding this comment

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

this line is in here twice


private static final String TAG = "TCPTransportManager";

TCPHandler tcpHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be local

Copy link
Contributor

Choose a reason for hiding this comment

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

(unless we re-use it in resetSession() in which case it could be private

@joeygrover
Copy link
Member Author

@BrettyWhite Updates made based on comments, ready for review

@BrettyWhite BrettyWhite merged commit 6582a36 into develop Aug 27, 2019
@BrettyWhite BrettyWhite deleted the feature/tcp_transportManager branch August 27, 2019 17:34
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.

3 participants