Dev nearby - #11
Conversation
| } | ||
| } | ||
|
|
||
| private fun checkAndInitiateConnections(endpointMap: Map<String, EndpointInfo>) { |
There was a problem hiding this comment.
There is no reason for this to be a separate function. It is a private function, not very long, and only ever called by line 220 (observeEndpointStatusFlow). I think this makes the code harder to read/follow.
| startDiscovery() | ||
| } | ||
|
|
||
| fun stop() { |
There was a problem hiding this comment.
This should be close. It should also use an atomic boolean to make sure that even if close is called multiple times, this function should only be run once:
e.g.
val isClosed = AtomicBoolean(false)
if(!isClosed.getAndSet(true)) {
//stop advertising discovery scope etc
}
There was a problem hiding this comment.
Close functions should be stable even if they are called multiple times - hence you need the atomic boolean.
|
|
||
| import java.nio.ByteBuffer | ||
|
|
||
| data class NearbyStreamHeader( |
There was a problem hiding this comment.
This should extend MmcpMessage - we also need the info included in it / virtual packet e.g. from address, to address, etc.
| fun toBytes(): ByteArray { | ||
| return ByteBuffer.allocate(12).apply { | ||
| putInt(streamId) | ||
| put(if (isReply) 1.toByte() else 0.toByte()) |
There was a problem hiding this comment.
no need for the if statement - Boolean.toByte() will already do the same
| private fun handleStreamPayload(endpointId: String, payload: Payload) { | ||
| checkClosed() | ||
| payload.asStream()?.asInputStream()?.use { inputStream -> | ||
| val headerBytes = ByteArray(12) |
There was a problem hiding this comment.
By convention never use any literal hardcoded like this other than 0, 1, true, and false. This should be defined as a single constant. If something was added this will get bigger. There should be one and only one place that should be changed.
Important principle - DRY - Dont Repeat Yourself / use a single point of truth.
|
|
||
| // Establish the VPN interface | ||
| vpnInterface = builder.establish() | ||
| vpnInterface?.let { |
There was a problem hiding this comment.
This is silently failing - as per https://github.com/UstadMobile/UstadMobile/blob/primary/CODING-STYLE.md "Do not use null checks that fail silently and would lead to code that doesn't behave as expect with no logging output or exception" - if vpninterface was null - this would fail silently.
- Updated dependencies to match versions used in Ustad.
- Updated dependencies to match versions used in Ustad.
… for IP addresses. - Replaced direct `Thread` usage with `ExecutorService` for better thread management. - Ensured all IP address handling uses `InetAddress` instead of raw `String`. - Refactored header parsing functions to handle exceptions and follow coding conventions.
| private const val IPPROTO_ICMPV6 = 58 // ICMPv6 protocol number | ||
|
|
||
| // Constants for byte offsets and field lengths | ||
| private const val IPV4_PROTOCOL_OFFSET = 9 |
There was a problem hiding this comment.
Needs a comment on each constant explaining why that is the value or link to reference.
| * @param length the length of the packet data. | ||
| */ | ||
| private fun processPacket(buffer: ByteBuffer, length: Int) { | ||
| buffer.flip() |
There was a problem hiding this comment.
Why flip it? Needs an explanation / reference as to why that is being done.
| private fun parseIPv6Header(buffer: ByteBuffer): Pair<Int, String> { | ||
| try { | ||
| val protocol = buffer.get(IPV6_PROTOCOL_OFFSET).toInt() | ||
| val isTcp = protocol == IPPROTO_TCP |
There was a problem hiding this comment.
Protocol: would be better to use an enum for this because there are only three valid values.
| * @param buffer the buffer containing the IPv6 packet data. | ||
| * @return a pair of the destination port and the protocol name. | ||
| */ | ||
| private fun parseIPv6Header(buffer: ByteBuffer): Pair<Int, String> { |
There was a problem hiding this comment.
parseIpHeader functions should return a data class (e.g. IpHeader) - that should contain destination address (InetAddress), destination port, origin address (InetAddress), origin port, and protocol.
There was a problem hiding this comment.
Note we need to know the origin port because there could be multiple streams going to the same destination. For Meshrabiya to match things up correctly, we will need to know the origin.
…d protocol handling with enum, and modified parseIpHeader to return a data class with origin details
…er according to the changes in NearbyVirtualNetwork. Added a method in OriginatingMessageManager to get the list of virtual IP addresses.
- Modify OriginatingMessageManager to handle multiple addresses
- Modify OriginatingMessageManager to handle multiple addresses
- Modify OriginatingMessageManager to handle multiple addresses
- Modify OriginatingMessageManager to handle multiple addresses
- Modify OriginatingMessageManager to handle multiple addresses
- Modify OriginatingMessageManager to handle multiple addresses
- Added virtualNode.addVirtualNetworkInterface to NearbyTestViewModel. - Fixed issues in NearbyVirtualNetwork class and added comments for function clarity. - Added addVirtualNetworkInterface function in VirtualNode.kt.
|
|
||
| private fun initializeNearbyNetwork() { | ||
| try { | ||
| nearbyNetwork = NearbyVirtualNetwork( |
There was a problem hiding this comment.
This should not be done here. Initializing the virtual network every time a viewmodel starts is obviously wrong.
2 participants
This PR adds better handling for IP addresses and refines the MMCP Ping and Pong message flow. It also includes improvements in connection management.