From f663b1d96276021724b57c5526d92db578eebbe8 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Thu, 19 Jun 2025 12:43:55 -0400 Subject: [PATCH 01/10] Primary changes: network interface selection and directed broadcast address These are the intended main changes for the Bloom Wi-Fi improvement effort. They address two things: (1) incorrect local IP address can sometimes be advertised, and (2) the advertisement should go out on a *directed* broadcast address, not the limited broadcast address (all FFs). Some debug output is still present. I will remove it before making a PR. --- .../BloomPub/wifi/BloomReaderUDPListener.cs | 77 ++- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 457 ++++++++++++++++-- 2 files changed, 483 insertions(+), 51 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs index 5c5338fd5210..3b576a52b71d 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs @@ -2,6 +2,7 @@ using System.Diagnostics; using System.Net; using System.Net.Sockets; +using System.Text; // debug only, for Encoding.ASCII.GetString() using System.Threading; using Bloom.Publish.BloomPUB.wifi; @@ -35,35 +36,81 @@ public BloomReaderUDPListener() /// public void ListenForUDPPackages() { + // UdpClient needs a constructor that specifies not just the port but also the + // IP address. + // + // If we specify the port only, this article describes how the system proceeds: + // https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.udpclient.-ctor?view=net-9.0 + // "This constructor creates an underlying Socket and binds it to the port number + // from which you intend to communicate. Use this constructor if you are only + // interested in setting the local port number. The underlying service provider + // will assign the local IP address." + // And, similar to what has been observed in Advertiser, the "underlying service + // provider" sometimes assigns the IP address from the wrong network interface. + // + // So, create the endpoint first setting its port *and* IP address. Then pass that + // endpoint into the UdpClient constructor. + + IPEndPoint groupEP = null; + try { - _listener = new UdpClient(_portToListen); + groupEP = new IPEndPoint(IPAddress.Any, _portToListen); + + if (groupEP == null) + { + Debug.WriteLine("UDPListener, ERROR creating IPEndPoint, bail"); + return; + } + + _listener = new UdpClient(groupEP); + + if (_listener == null) + { + Debug.WriteLine("UDPListener, ERROR creating UdpClient, bail"); + return; + } } catch (SocketException e) { //log then do nothing + Debug.WriteLine("UDPListener, SocketException-1 = " + e); Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); } - if (_listener != null) - { - IPEndPoint groupEP = new IPEndPoint(IPAddress.Any, 0); + // Local endpoint has been created on the port that BloomReader will respond to. + // And the endpoint's address 'IPAddress.Any' means that *all* network interfaces + // on this machine will be monitored for UDP packets sent to the designated port. - while (_listening) + while (_listening) + { + try { - try + // Log our local address and port. + if (_listener?.Client?.LocalEndPoint is IPEndPoint localEP) { - byte[] bytes = _listener.Receive(ref groupEP); // waits for packet from Android. - - //raise event - NewMessageReceived?.Invoke(this, new AndroidMessageArgs(bytes)); + Debug.WriteLine("UDP listening will wait for packet on {0}, port {1}", localEP.Address, localEP.Port); } - catch (SocketException se) + + byte[] bytes = _listener.Receive(ref groupEP); // waits for packet from Android. + + // DEBUG ONLY + Debug.WriteLine("WM, UDPListener, got {0} bytes (raising \'NewMessageReceived\'):", bytes.Length); + var bytesToString = Encoding.ASCII.GetString(bytes, 0, bytes.Length); + Debug.WriteLine(" " + bytesToString.Substring(0, bytes.Length)); + // END DEBUG + + //raise event + NewMessageReceived?.Invoke(this, new AndroidMessageArgs(bytes)); + } + catch (SocketException se) + { + Debug.WriteLine("UDPListener, SocketException-2 = " + se); + if (!_listening || se.SocketErrorCode == SocketError.Interrupted) { - if (!_listening || se.SocketErrorCode == SocketError.Interrupted) - return; // no problem, we're just closing up shop - throw se; + return; // no problem, we're just closing up shop } + throw se; } } } @@ -78,7 +125,9 @@ public void StopListener() } if (_listeningThread == null) + { return; + } // Since we told the listener to close already this shouldn't have to do much (nor be dangerous) _listeningThread.Abort(); diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index eace2dfe3059..1f3873542b4b 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -1,8 +1,10 @@ -using System; +using System; using System.Diagnostics; using System.Linq; using System.Net; +using System.Net.NetworkInformation; using System.Net.Sockets; +using System.Runtime.InteropServices; using System.Text; using System.Threading; using Bloom.Api; @@ -33,16 +35,78 @@ public string BookVersion } public string TitleLanguage; - private UdpClient _client; + private UdpClient _clientBroadcast; private Thread _thread; - private IPEndPoint _endPoint; + private IPEndPoint _localEP; + private IPEndPoint _remoteEP; + private string _localIp = ""; + private string _remoteIp = ""; // will hold UDP broadcast address + private string _subnetMask = ""; + private string _currentIpAddress = ""; + private string _cachedIpAddress = ""; + + // Layout of a row in the IPv4 routing table. + [StructLayout(LayoutKind.Sequential)] + public struct MIB_IPFORWARDROW + { + public uint dwForwardDest; + public uint dwForwardMask; + public uint dwForwardPolicy; + public uint dwForwardNextHop; + public int dwForwardIfIndex; + public int dwForwardType; + public int dwForwardProto; + public int dwForwardAge; + public int dwForwardNextHopAS; + public int dwForwardMetric1; // the "interface metric" we need + public int dwForwardMetric2; + public int dwForwardMetric3; + public int dwForwardMetric4; + public int dwForwardMetric5; + } + + // Hold a copy of the IPv4 routing table, which we will examine + // to find which row/route has the lowest "interface metric". + [StructLayout(LayoutKind.Sequential)] + private struct MIB_IPFORWARDTABLE + { + private int dwNumEntries; + private MIB_IPFORWARDROW table; + } + + // We use an unmanaged function in the C/C++ DLL "iphlpapi.dll". + // - "true": calling this function *can* set an error code, + // which will be retrieveable via Marshal.GetLastWin32Error() + [DllImport("iphlpapi.dll", SetLastError = true)] + static extern int GetIpForwardTable(IntPtr pIpForwardTable, ref int pdwSize, bool bOrder); + + // Hold relevant network interface attributes. + private class InterfaceInfo + { + public string IpAddr { get; set; } + public string Description { get; set; } + public string NetMask { get; set; } + public int Metric { get; set; } + } + + // Hold the current network interface candidates, one for Wi-Fi and one + // for Ethernet. + private InterfaceInfo IfaceWifi = new InterfaceInfo(); + private InterfaceInfo IfaceEthernet = new InterfaceInfo(); + + // Possible results from network interface assessment. + private enum CommTypeToExpect + { + None = 0, + WiFi = 1, + Ethernet = 2 + } // The port on which we advertise. // ChorusHub uses 5911 to advertise. Bloom looks for a port for its server at 8089 and 10 following ports. // https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers shows a lot of ports in use around 8089, // but nothing between 5900 and 5931. Decided to use a number similar to ChorusHub. - private const int Port = 5913; // must match port in BloomReader NewBookListenerService.startListenForUDPBroadcast - private string _currentIpAddress; + private const int _portForBroadcast = 5913; // must match port in BloomReader NewBookListenerService.startListenForUDPBroadcast private byte[] _sendBytes; // Data we send in each advertisement packet private readonly WebSocketProgress _progress; @@ -53,12 +117,6 @@ internal WiFiAdvertiser(WebSocketProgress progress) public void Start() { - // The doc seems to indicate that EnableBroadcast is required for doing broadcasts. - // In practice it seems to be required on Mono but not on Windows. - // This may be fixed in a later version of one platform or the other, but please - // test both if tempted to remove it. - _client = new UdpClient { EnableBroadcast = true }; - _endPoint = new IPEndPoint(IPAddress.Parse("255.255.255.255"), Port); _thread = new Thread(Work); _thread.Start(); } @@ -71,28 +129,123 @@ private void Work() idSuffix: "beginAdvertising", message: "Advertising book to Bloom Readers on local network..." ); + + // We must be confident that the local IP address we advertise *in* the UDP broadcast + // packet is the same one the network stack will use *for* the broadcast. Gleaning the + // local IP address from a UdpClient usually yields the correct one, but unfortunately + // it can be different on some machines. When that happens the remote Android gets the + // wrong address from the advert, and Desktop won't get the Android book request. + // + // To mitigate, change how the UdpClient is instantiated. Assign it the IP address of + // the network interface the network stack will use: the interface having the lowest + // "interface metric." + // + // The PC on which this runs likely has both WiFi and Ethernet. Either can work, + // but preference is given to WiFi. The reason: although this PC can likely go either + // way, the Android device only has WiFi. For the book transfer to work both Desktop + // and Reader must be on the same subnet. If Desktop is using Ethernet it may not be + // on the same subnet as Reader, especially on larger networks. The chances that both + // PC and Android are on the same subnet are greatest if both are using WiFi. + + // Examine the network interfaces and determine which will be used for network traffic. + // Candidates will get stored in the two results objects. + CommTypeToExpect ifcResult = GetInterfaceStackWillUse(); + + if (ifcResult == CommTypeToExpect.None) + { + Debug.WriteLine("WiFiAdvertiser, local IP not found"); + return; + } + + string ifaceDesc = ""; + + if (ifcResult == CommTypeToExpect.WiFi) + { + // Network stack will use WiFi. + _localIp = IfaceWifi.IpAddr; + _subnetMask = IfaceWifi.NetMask; + ifaceDesc = IfaceWifi.Description; + } + else + { + // Network stack will use Ethernet. + _localIp = IfaceEthernet.IpAddr; + _subnetMask = IfaceEthernet.NetMask; + ifaceDesc = IfaceEthernet.Description; + } + + // Now that we know the IP address and subnet mask in effect, calculate + // the broadcast address to use. + _remoteIp = GetDirectedBroadcastAddress(_localIp, _subnetMask); + if (_remoteIp.Length == 0) + { + Debug.WriteLine("WiFiAdvertiser, ERROR: can't get broadcast address, bail"); + return; + } + try { + // Instantiate UdpClient using the local IP address we just got. + IPEndPoint epBroadcast = null; + + epBroadcast = new IPEndPoint(IPAddress.Parse(_localIp), _portForBroadcast); + if (epBroadcast == null) + { + Debug.WriteLine("WiFiAdvertiser, ERROR creating IPEndPoint, bail"); + return; + } + + _clientBroadcast = new UdpClient(epBroadcast); + if (_clientBroadcast == null) + { + Debug.WriteLine("WiFiAdvertiser, ERROR creating UdpClient, bail"); + return; + } + + // The doc seems to indicate that EnableBroadcast is required for doing broadcasts. + // In practice it seems to be required on Mono but not on Windows. + // This may be fixed in a later version of one platform or the other, but please + // test both if tempted to remove it. + _clientBroadcast.EnableBroadcast = true; + + // Set up destination endpoint. + _remoteEP = new IPEndPoint(IPAddress.Parse(_remoteIp), _portForBroadcast); + + // Log key data for tech support. + Debug.WriteLine("UDP advertising will use: _localIp = {0}:{1} ({2})", _localIp, epBroadcast.Port, ifaceDesc); + Debug.WriteLine(" _subnetMask = " + _subnetMask); + Debug.WriteLine(" _remoteIp = {0}:{1}", _remoteEP.Address, _remoteEP.Port); + + // Local and remote are ready. Advertise once per second, indefinitely. while (true) { if (!Paused) { UpdateAdvertisementBasedOnCurrentIpAddress(); - _client.BeginSend( + + Debug.WriteLine("WiFiAdvertiser, broadcasting advert to: {0}:{1}", _remoteEP.Address, _remoteEP.Port); // TEMPORARY! + _clientBroadcast.BeginSend( _sendBytes, _sendBytes.Length, - _endPoint, + _remoteEP, SendCallback, - _client + _clientBroadcast ); } Thread.Sleep(1000); } } + catch (SocketException e) + { + Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); + // Log it. + Debug.WriteLine("WiFiAdvertiser::Work, SocketException: " + e); + // Don't know what _progress.Message() is desired here, add as appropriate. + } catch (ThreadAbortException) { _progress.Message(idSuffix: "Stopped", message: "Stopped Advertising."); - _client.Close(); + _clientBroadcast.Close(); } catch (Exception error) { @@ -112,9 +265,11 @@ public static void SendCallback(IAsyncResult args) { } /// private void UpdateAdvertisementBasedOnCurrentIpAddress() { - if (_currentIpAddress != GetLocalIpAddress()) + _currentIpAddress = _localIp; + + if (_cachedIpAddress != _currentIpAddress) { - _currentIpAddress = GetLocalIpAddress(); + _cachedIpAddress = _currentIpAddress; dynamic advertisement = new DynamicJson(); advertisement.title = BookTitle; advertisement.version = BookVersion; @@ -122,39 +277,267 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() advertisement.protocolVersion = WiFiPublisher.ProtocolVersion; advertisement.sender = System.Environment.MachineName; + // If we do eventually add capability to display the advert as a QR code, + // the local IP address will need to be included. Might as well add it now. + advertisement.senderIP = _currentIpAddress; + _sendBytes = Encoding.UTF8.GetBytes(advertisement.ToString()); //EventLog.WriteEntry("Application", "Serving at http://" + _currentIpAddress + ":" + ChorusHubOptions.MercurialPort, EventLogEntryType.Information); } } - /// - /// The intent here is to get an IP address by which this computer can be found on the local subnet. - /// This is ambiguous if the computer has more than one IP address (typically for an Ethernet and WiFi adapter). - /// Early experiments indicate that things work whichever one is used, assuming the networks are connected. - /// Eventually we may want to prefer WiFi if available (see code in HearThis), or even broadcast on all of them. - /// - /// - private string GetLocalIpAddress() + // Survey the network interfaces and determine which one, if any, the network + // stack will use for network traffic. + // - During the assessment the current leading WiFi candidate will be held in + // 'IfaceWifi', and similarly the current best candidate for Ethernet will + // be in 'IfaceEthernet'. + // - After assessment inform calling code of the winner by returning an enum + // indicating which of the candidate structs to draw from: WiFi, Ethernet, + // or neither. + // + private CommTypeToExpect GetInterfaceStackWillUse() + { + int currentIfaceMetric; + + // Initialize result structs metric field to the highest possible value + // so the first interface metric value seen will always replace it. + IfaceWifi.Metric = int.MaxValue; + IfaceEthernet.Metric = int.MaxValue; + + // Retrieve all network interfaces that are *active*. + var allOperationalNetworks = NetworkInterface.GetAllNetworkInterfaces() + .Where(ni => ni.OperationalStatus == OperationalStatus.Up).ToArray(); + + if (!allOperationalNetworks.Any()) + { + Debug.WriteLine("WiFiAdvertiser, ERROR, no network interfaces are operational"); + return CommTypeToExpect.None; + } + + // Get key attributes of active network interfaces. + foreach (NetworkInterface ni in allOperationalNetworks) + { + // If we can't get IP or IPv4 properties for this interface, skip it. + var ipProps = ni.GetIPProperties(); + if (ipProps == null) + { + continue; + } + var ipv4Props = ipProps.GetIPv4Properties(); + if (ipv4Props == null) + { + continue; + } + + foreach (UnicastIPAddressInformation ip in ipProps.UnicastAddresses) + { + // We don't consider IPv6 so filter for IPv4 ('InterNetwork')... + if (ip.Address.AddressFamily == AddressFamily.InterNetwork) + { + // ...And of these we care only about WiFi and Ethernet. + if (ni.NetworkInterfaceType == NetworkInterfaceType.Wireless80211) + { + currentIfaceMetric = GetMetricForInterface(ipv4Props.Index); + + // Save this interface if its metric is lowest we've seen so far. + if (currentIfaceMetric < IfaceWifi.Metric) + { + IfaceWifi.IpAddr = ip.Address.ToString(); + IfaceWifi.NetMask = ip.IPv4Mask.ToString(); + IfaceWifi.Description = ni.Description; + IfaceWifi.Metric = currentIfaceMetric; + } + } + else if (ni.NetworkInterfaceType == NetworkInterfaceType.Ethernet) + { + currentIfaceMetric = GetMetricForInterface(ipv4Props.Index); + + // Save this interface if its metric is lowest we've seen so far. + if (currentIfaceMetric < IfaceEthernet.Metric) + { + IfaceEthernet.IpAddr = ip.Address.ToString(); + IfaceEthernet.NetMask = ip.IPv4Mask.ToString(); + IfaceEthernet.Description = ni.Description; + IfaceEthernet.Metric = currentIfaceMetric; + } + } + } + } + } + + // Active network interfaces have all been assessed. + // - The WiFi interface having the lowest metric has been saved in the + // WiFi result struct. Note: if no active WiFi interface was seen then + // the result struct's metric field will still have its initial value. + // - Likewise for Ethernet. + // Now choose the winner, if there is one: + // - If we saw an active WiFi interface, return that + // - Else if we saw an active Ethernet interface, return that + // - Else there is no winner so return none + if (IfaceWifi.Metric < int.MaxValue) + { + return CommTypeToExpect.WiFi; + } + if (IfaceEthernet.Metric < int.MaxValue) + { + return CommTypeToExpect.Ethernet; + } + + Debug.WriteLine("UDP advertising: no suitable network interface found"); + return CommTypeToExpect.None; + } + + // Get a key piece of info ("metric") from the specified network interface. + // https://learn.microsoft.com/en-us/windows/win32/api/iphlpapi/nf-iphlpapi-getipforwardtable + // + // Retrieving the metric is not as simple as grabbing one of the fields in + // the network interface. The metric resides in the network stack routing + // table. One of the interface fields ("Index") is also in the routing table + // and is how we correlate the two. + // - Calling code (walking the interface collection) passes in the index + // of the interface whose "best" metric it wants. + // - This function walks the routing table looking for all rows (each of + // which is a route) containing that index. It notes the metric in each + // route and returns the lowest among all routes/rows for the interface. + // + int GetMetricForInterface(int interfaceIndex) { - string localIp = null; - var host = Dns.GetHostEntry(Dns.GetHostName()); + // Initialize to "worst" possible metric (Win10 Pro: 2^31 - 1). + // It can only get better from there! + int bestMetric = int.MaxValue; + + // Preliminary: call with a null buffer ('size') to learn how large a + // buffer is needed to hold a copy of the routing table. + int size = 0; + GetIpForwardTable(IntPtr.Zero, ref size, false); + + IntPtr tableBuf; + + try + { + // 'size' now shows how large a buffer is needed, so allocate it. + tableBuf = Marshal.AllocHGlobal(size); + } + catch (OutOfMemoryException e) + { + Debug.WriteLine(" GetMetricForInterface, ERROR creating buffer: " + e); + return bestMetric; + } - foreach ( - var ipAddress in host.AddressList.Where( - ipAddress => ipAddress.AddressFamily == AddressFamily.InterNetwork - ) - ) + try { - if (localIp != null) + // Copy the routing table into buffer for examination. + int error = GetIpForwardTable(tableBuf, ref size, false); + if (error != 0) { - if (host.AddressList.Length > 1) + // Something went wrong so bail. + // It is tempting to add a dealloc call here, but don't. The + // dealloc in the 'finally' block *will* be done (I checked). + Debug.WriteLine(" GetMetricForInterface, ERROR, GetIpForwardTable() = {0}, returning {1}", error, bestMetric); + return bestMetric; + } + + // Get number of routing table entries. + int numEntries = Marshal.ReadInt32(tableBuf); + + // Advance pointer past the integer to point at 1st row. + IntPtr rowPtr = IntPtr.Add(tableBuf, 4); + + // Walk the routing table looking for rows involving the the network + // interface passed in. For each such row/route, check the metric. + // If it is lower than the lowest we've yet seen, save it to become + // the new benchmark. + for (int i = 0; i < numEntries; i++) + { + MIB_IPFORWARDROW row = Marshal.PtrToStructure(rowPtr); + if (row.dwForwardIfIndex == interfaceIndex) { - //EventLog.WriteEntry("Application", "Warning: this machine has more than one IP address", EventLogEntryType.Warning); + bestMetric = Math.Min(bestMetric, row.dwForwardMetric1); } + rowPtr = IntPtr.Add(rowPtr, Marshal.SizeOf()); + } + } + catch (Exception e) + { + if (e is AccessViolationException || e is MissingMethodException) + { + Debug.WriteLine(" GetMetricForInterface, ERROR: " + e); + } + } + finally + { + Marshal.FreeHGlobal(tableBuf); + } + + return bestMetric; + } + + // Construct a network interface's directed broadcast address. + // + // This is different from 255.255.255.255, the "limited broadcast address" which + // applies only to 0.0.0.0, the "zero network" beyond which broadcasts will never + // be propagated (https://en.wikipedia.org/wiki/Broadcast_address). + // But a directed broadcast CAN be forwarded to other subnets, if the network's + // routers are configured to allow it. They often aren't but at least the potential + // is there. For a thorough explanation see: + // https://www.practicalnetworking.net/stand-alone/local-broadcast-vs-directed-broadcast/ + // + // So, to maximize the chances that a Reader will hear book adverts we use the + // directed broadcast address. It is generated from an interface's IP address and + // subnet mask, both of which are passed in to this function. + // + // Bit-wise rationale: + // The subnet mask indicates how to handle the IP address bits. + // - '1' mask bits: the "network address" portion of the IP address. + // The broadcast address aims at the same network so just copy the IP + // address bits into the same positions of the broadcast address. + // 'ORing' IP bits with 1-mask-bits-inverted-to-0 keeps them all + // unchanged. + // - '0' mask bits: the "host ID" portion of the IP address. We want to + // fill this portion of the broadcast address with '1's so all hosts on + // the subnet will see the transmission. + // 'ORing' IP bits with 0-mask-bits-inverted-to-1 makes them all 1. + // + // Function operation: + // convert IP address and subnet mask strings to byte arrays + // create byte array to hold broadcast address result + // FOR each IP address octet and corresponding subnet mask octet, starting with most significant + // compute broadcast address octet per "Bit-wise rationale" above + // END + // convert broadcast address byte array to IP address string and return it + // + // Note: the local IP and mask inputs are not explicitly checked. Any issues they + // may have will become apparent by the catch block firing. + // + private string GetDirectedBroadcastAddress(string ipIn, string maskIn) + { + try + { + IPAddress ipAddress = IPAddress.Parse(ipIn); + IPAddress subnetMask = IPAddress.Parse(maskIn); + byte[] ipBytes = ipAddress.GetAddressBytes(); + byte[] maskBytes = subnetMask.GetAddressBytes(); + + if (ipBytes.Length != maskBytes.Length) + { + Console.WriteLine("CalculateBroadcastAddress, ERROR, length mismatch, IP vs mask: {0}, {1}", + ipBytes.Length, maskBytes.Length); + } + + byte[] bcastBytes = new byte[ipBytes.Length]; + for (int i = 0; i < ipBytes.Length; i++) + { + //bcastBytes[i] = (byte)(ipBytes[i] | (maskBytes[i] ^ 255)); + bcastBytes[i] = (byte)(ipBytes[i] | ~maskBytes[i]); } - localIp = ipAddress.ToString(); + + return new IPAddress(bcastBytes).ToString(); + } + catch (Exception) + { + // Invalid IP address or subnet mask. + return ""; } - return localIp ?? "Could not determine IP Address!"; } public void Stop() From 9e06a5003ae2ce833333b42ad8c7e9d157e08155 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Mon, 23 Jun 2025 12:52:16 -0400 Subject: [PATCH 02/10] Code cleanup for PR Remove some debugs, modify others. Works on wired connection at home, next is to test on Wi-Fi at JAARS. --- .../BloomPub/wifi/BloomReaderUDPListener.cs | 8 +++----- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 20 +++++++++---------- 2 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs index 3b576a52b71d..e629f954f973 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs @@ -74,7 +74,6 @@ public void ListenForUDPPackages() catch (SocketException e) { //log then do nothing - Debug.WriteLine("UDPListener, SocketException-1 = " + e); Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); } @@ -95,9 +94,9 @@ public void ListenForUDPPackages() byte[] bytes = _listener.Receive(ref groupEP); // waits for packet from Android. // DEBUG ONLY - Debug.WriteLine("WM, UDPListener, got {0} bytes (raising \'NewMessageReceived\'):", bytes.Length); - var bytesToString = Encoding.ASCII.GetString(bytes, 0, bytes.Length); - Debug.WriteLine(" " + bytesToString.Substring(0, bytes.Length)); + //Debug.WriteLine("UDPListener, got {0} bytes (raising \'NewMessageReceived\'):", bytes.Length); + //var bytesToString = Encoding.ASCII.GetString(bytes, 0, bytes.Length); + //Debug.WriteLine(" " + bytesToString.Substring(0, bytes.Length)); // END DEBUG //raise event @@ -105,7 +104,6 @@ public void ListenForUDPPackages() } catch (SocketException se) { - Debug.WriteLine("UDPListener, SocketException-2 = " + se); if (!_listening || se.SocketErrorCode == SocketError.Interrupted) { return; // no problem, we're just closing up shop diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 1f3873542b4b..09892a9c8162 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -153,7 +153,7 @@ private void Work() if (ifcResult == CommTypeToExpect.None) { - Debug.WriteLine("WiFiAdvertiser, local IP not found"); + Debug.WriteLine("WiFiAdvertiser, ERROR getting local IP"); return; } @@ -179,7 +179,7 @@ private void Work() _remoteIp = GetDirectedBroadcastAddress(_localIp, _subnetMask); if (_remoteIp.Length == 0) { - Debug.WriteLine("WiFiAdvertiser, ERROR: can't get broadcast address, bail"); + Debug.WriteLine("WiFiAdvertiser, ERROR getting broadcast address"); return; } @@ -191,14 +191,14 @@ private void Work() epBroadcast = new IPEndPoint(IPAddress.Parse(_localIp), _portForBroadcast); if (epBroadcast == null) { - Debug.WriteLine("WiFiAdvertiser, ERROR creating IPEndPoint, bail"); + Debug.WriteLine("WiFiAdvertiser, ERROR creating IPEndPoint"); return; } _clientBroadcast = new UdpClient(epBroadcast); if (_clientBroadcast == null) { - Debug.WriteLine("WiFiAdvertiser, ERROR creating UdpClient, bail"); + Debug.WriteLine("WiFiAdvertiser, ERROR creating UdpClient"); return; } @@ -223,7 +223,7 @@ private void Work() { UpdateAdvertisementBasedOnCurrentIpAddress(); - Debug.WriteLine("WiFiAdvertiser, broadcasting advert to: {0}:{1}", _remoteEP.Address, _remoteEP.Port); // TEMPORARY! + //Debug.WriteLine("WiFiAdvertiser, broadcasting advert to: {0}:{1}", _remoteEP.Address, _remoteEP.Port); // TEMPORARY! _clientBroadcast.BeginSend( _sendBytes, _sendBytes.Length, @@ -383,7 +383,7 @@ private CommTypeToExpect GetInterfaceStackWillUse() return CommTypeToExpect.Ethernet; } - Debug.WriteLine("UDP advertising: no suitable network interface found"); + Debug.WriteLine("WiFiAdvertiser, ERROR, no suitable network interface found"); return CommTypeToExpect.None; } @@ -420,7 +420,7 @@ int GetMetricForInterface(int interfaceIndex) } catch (OutOfMemoryException e) { - Debug.WriteLine(" GetMetricForInterface, ERROR creating buffer: " + e); + Debug.WriteLine("GetMetricForInterface, ERROR creating buffer: " + e); return bestMetric; } @@ -433,7 +433,7 @@ int GetMetricForInterface(int interfaceIndex) // Something went wrong so bail. // It is tempting to add a dealloc call here, but don't. The // dealloc in the 'finally' block *will* be done (I checked). - Debug.WriteLine(" GetMetricForInterface, ERROR, GetIpForwardTable() = {0}, returning {1}", error, bestMetric); + Debug.WriteLine("GetMetricForInterface, ERROR, GetIpForwardTable() = {0}, returning {1}", error, bestMetric); return bestMetric; } @@ -461,7 +461,7 @@ int GetMetricForInterface(int interfaceIndex) { if (e is AccessViolationException || e is MissingMethodException) { - Debug.WriteLine(" GetMetricForInterface, ERROR: " + e); + Debug.WriteLine("GetMetricForInterface, ERROR: " + e); } } finally @@ -522,12 +522,12 @@ private string GetDirectedBroadcastAddress(string ipIn, string maskIn) { Console.WriteLine("CalculateBroadcastAddress, ERROR, length mismatch, IP vs mask: {0}, {1}", ipBytes.Length, maskBytes.Length); + return ""; } byte[] bcastBytes = new byte[ipBytes.Length]; for (int i = 0; i < ipBytes.Length; i++) { - //bcastBytes[i] = (byte)(ipBytes[i] | (maskBytes[i] ^ 255)); bcastBytes[i] = (byte)(ipBytes[i] | ~maskBytes[i]); } From 08a00c9bde8504ca6324c2416d814e6fc653af04 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Thu, 30 Oct 2025 15:42:19 -0400 Subject: [PATCH 03/10] Changes round 1 per PR review, all except advert loop All necessary (IMO) changes except restructuring of the advertising loop. That will be next commit. Changes in this one include: - converting Debug.Writeline() to EventLog.WriteEntry() - remove CommTypeToExpect and fix its usages - rename some variables, improve some comments - a few other streamlining changes --- .../BloomPub/wifi/BloomReaderUDPListener.cs | 39 +++-- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 157 ++++++++---------- 2 files changed, 92 insertions(+), 104 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs index e629f954f973..520e777eee9d 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs @@ -19,7 +19,11 @@ class BloomReaderUDPListener private int _portToListen = 5915; Thread _listeningThread; public event EventHandler NewMessageReceived; - UdpClient _listener = null; + + // Client by which we receive replies to broadcast book advert. It listens on all network + // interfaces on the expected listening port. + UdpClient _clientForBookRequestReceive = null; + private bool _listening; //constructor: starts listening. @@ -36,8 +40,7 @@ public BloomReaderUDPListener() /// public void ListenForUDPPackages() { - // UdpClient needs a constructor that specifies not just the port but also the - // IP address. + // UdpClient needs a constructor that specifies more than just the port. // // If we specify the port only, this article describes how the system proceeds: // https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.udpclient.-ctor?view=net-9.0 @@ -48,8 +51,11 @@ public void ListenForUDPPackages() // And, similar to what has been observed in Advertiser, the "underlying service // provider" sometimes assigns the IP address from the wrong network interface. // - // So, create the endpoint first setting its port *and* IP address. Then pass that - // endpoint into the UdpClient constructor. + // So, create the endpoint first, setting its port *and* special IP addr 'IPAddress.Any'. + // This address causes the endpoint to listen for client activity on all network interfaces, + // bypassing the possibility of the network stack choosing a wrong address: + // https://learn.microsoft.com/en-us/dotnet/api/system.net.ipaddress.any?view=net-9.0 + // Then base the UdpClient on this endpoint. IPEndPoint groupEP = null; @@ -59,15 +65,15 @@ public void ListenForUDPPackages() if (groupEP == null) { - Debug.WriteLine("UDPListener, ERROR creating IPEndPoint, bail"); + EventLog.WriteEntry("Application", "UDPListener, ERROR creating IPEndPoint"); return; } - _listener = new UdpClient(groupEP); + _clientForBookRequestReceive = new UdpClient(groupEP); - if (_listener == null) + if (_clientForBookRequestReceive == null) { - Debug.WriteLine("UDPListener, ERROR creating UdpClient, bail"); + EventLog.WriteEntry("Application", "UDPListener, ERROR creating UdpClient"); return; } } @@ -77,21 +83,20 @@ public void ListenForUDPPackages() Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); } - // Local endpoint has been created on the port that BloomReader will respond to. - // And the endpoint's address 'IPAddress.Any' means that *all* network interfaces - // on this machine will be monitored for UDP packets sent to the designated port. + // Listener has been created on the port that BloomReader will respond to, and + // will monitor *all* network interfaces for UDP packets sent to that port. while (_listening) { try { // Log our local address and port. - if (_listener?.Client?.LocalEndPoint is IPEndPoint localEP) + if (_clientForBookRequestReceive?.Client?.LocalEndPoint is IPEndPoint localEP) { - Debug.WriteLine("UDP listening will wait for packet on {0}, port {1}", localEP.Address, localEP.Port); + EventLog.WriteEntry("Application", $"UDP listening will wait for packet on {localEP.Address}, port {localEP.Port}"); } - byte[] bytes = _listener.Receive(ref groupEP); // waits for packet from Android. + byte[] bytes = _clientForBookRequestReceive.Receive(ref groupEP); // waits for packet from Android. // DEBUG ONLY //Debug.WriteLine("UDPListener, got {0} bytes (raising \'NewMessageReceived\'):", bytes.Length); @@ -118,8 +123,8 @@ public void StopListener() if (_listening) { _listening = false; - _listener?.Close(); // forcibly end communication - _listener = null; + _clientForBookRequestReceive?.Close(); // forcibly end communication + _clientForBookRequestReceive = null; } if (_listeningThread == null) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 09892a9c8162..278efa07bba3 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -35,12 +35,21 @@ public string BookVersion } public string TitleLanguage; - private UdpClient _clientBroadcast; + // Client by which we send out broadcast book adverts. It uses our local IP address (attached + // to the correct network interface) and the expected port. + private UdpClient _clientForBookAdvertSend; + private Thread _thread; - private IPEndPoint _localEP; private IPEndPoint _remoteEP; - private string _localIp = ""; - private string _remoteIp = ""; // will hold UDP broadcast address + + // Our "local IP address" of the network interface used for Bloom network traffic: + // - it is the target address of incoming book requests from Androids + // - it is also used to derive the destination address for outgoing book advert broadcasts + private string _ipForReceivingReplies = ""; + + // IP address to which outgoing book advert broadcasts are sent, a "directed broadcast address": + private string _ipForSendingBroadcast = ""; + private string _subnetMask = ""; private string _currentIpAddress = ""; private string _cachedIpAddress = ""; @@ -89,19 +98,6 @@ private class InterfaceInfo public int Metric { get; set; } } - // Hold the current network interface candidates, one for Wi-Fi and one - // for Ethernet. - private InterfaceInfo IfaceWifi = new InterfaceInfo(); - private InterfaceInfo IfaceEthernet = new InterfaceInfo(); - - // Possible results from network interface assessment. - private enum CommTypeToExpect - { - None = 0, - WiFi = 1, - Ethernet = 2 - } - // The port on which we advertise. // ChorusHub uses 5911 to advertise. Bloom looks for a port for its server at 8089 and 10 following ports. // https://en.wikipedia.org/wiki/List_of_TCP_and_UDP_port_numbers shows a lot of ports in use around 8089, @@ -130,12 +126,13 @@ private void Work() message: "Advertising book to Bloom Readers on local network..." ); - // We must be confident that the local IP address we advertise *in* the UDP broadcast - // packet is the same one the network stack will use *for* the broadcast. Gleaning the - // local IP address from a UdpClient usually yields the correct one, but unfortunately - // it can be different on some machines. When that happens the remote Android gets the - // wrong address from the advert, and Desktop won't get the Android book request. - // + // We must ensure that the local IP address we advertise by the UDP broadcast is the + // same address from which the network stack makes the broadcast. Gleaning the local + // IP address from a UdpClient usually does yield the one that is being used, but + // unfortunately it can be different on some machines. When that happens the remote + // Android gets the wrong address from the advert, and Desktop won't get the Android + // book request. + // // To mitigate, change how the UdpClient is instantiated. Assign it the IP address of // the network interface the network stack will use: the interface having the lowest // "interface metric." @@ -148,57 +145,45 @@ private void Work() // PC and Android are on the same subnet are greatest if both are using WiFi. // Examine the network interfaces and determine which will be used for network traffic. - // Candidates will get stored in the two results objects. - CommTypeToExpect ifcResult = GetInterfaceStackWillUse(); + Debug.WriteLine("WM, Work, getting src and dest IPs"); // TEMPORARY + InterfaceInfo ifcResult = GetInterfaceStackWillUse(); - if (ifcResult == CommTypeToExpect.None) + if (ifcResult == null) { - Debug.WriteLine("WiFiAdvertiser, ERROR getting local IP"); + EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting local IP"); return; } - string ifaceDesc = ""; - - if (ifcResult == CommTypeToExpect.WiFi) - { - // Network stack will use WiFi. - _localIp = IfaceWifi.IpAddr; - _subnetMask = IfaceWifi.NetMask; - ifaceDesc = IfaceWifi.Description; - } - else - { - // Network stack will use Ethernet. - _localIp = IfaceEthernet.IpAddr; - _subnetMask = IfaceEthernet.NetMask; - ifaceDesc = IfaceEthernet.Description; - } + _ipForReceivingReplies = ifcResult.IpAddr; + _subnetMask = ifcResult.NetMask; + string ifaceDesc = ifcResult.Description; // Now that we know the IP address and subnet mask in effect, calculate - // the broadcast address to use. - _remoteIp = GetDirectedBroadcastAddress(_localIp, _subnetMask); - if (_remoteIp.Length == 0) + // the "directed" broadcast address to use. + _ipForSendingBroadcast = GetDirectedBroadcastAddress(_ipForReceivingReplies, _subnetMask); + if (_ipForSendingBroadcast.Length == 0) { - Debug.WriteLine("WiFiAdvertiser, ERROR getting broadcast address"); + EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting broadcast address"); return; } + Debug.WriteLine("WM, Work, got src and dest IPs"); // TEMPORARY try { - // Instantiate UdpClient using the local IP address we just got. - IPEndPoint epBroadcast = null; - - epBroadcast = new IPEndPoint(IPAddress.Parse(_localIp), _portForBroadcast); + Debug.WriteLine("WM, Work, try-begin"); // TEMPORARY + // Instantiate source endpoint using the local IP address we just got, + // then use that to create the UdpClient for broadcasting adverts. + IPEndPoint epBroadcast = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); if (epBroadcast == null) { - Debug.WriteLine("WiFiAdvertiser, ERROR creating IPEndPoint"); + EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating IPEndPoint"); return; } - _clientBroadcast = new UdpClient(epBroadcast); - if (_clientBroadcast == null) + _clientForBookAdvertSend = new UdpClient(epBroadcast); + if (_clientForBookAdvertSend == null) { - Debug.WriteLine("WiFiAdvertiser, ERROR creating UdpClient"); + EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating UdpClient"); return; } @@ -206,31 +191,33 @@ private void Work() // In practice it seems to be required on Mono but not on Windows. // This may be fixed in a later version of one platform or the other, but please // test both if tempted to remove it. - _clientBroadcast.EnableBroadcast = true; + _clientForBookAdvertSend.EnableBroadcast = true; // Set up destination endpoint. - _remoteEP = new IPEndPoint(IPAddress.Parse(_remoteIp), _portForBroadcast); + _remoteEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); // Log key data for tech support. - Debug.WriteLine("UDP advertising will use: _localIp = {0}:{1} ({2})", _localIp, epBroadcast.Port, ifaceDesc); - Debug.WriteLine(" _subnetMask = " + _subnetMask); - Debug.WriteLine(" _remoteIp = {0}:{1}", _remoteEP.Address, _remoteEP.Port); + EventLog.WriteEntry("Application", $"UDP advertising will use: _localIp = {_ipForReceivingReplies}:{epBroadcast.Port} ({ifaceDesc})"); + EventLog.WriteEntry("Application", $" _subnetMask = {_subnetMask}"); + EventLog.WriteEntry("Application", $" _remoteIp = {_remoteEP.Address}:{_remoteEP.Port}"); + Debug.WriteLine("WM, Work, try-end"); // TEMPORARY - // Local and remote are ready. Advertise once per second, indefinitely. + // Advertise once per second, indefinitely. while (true) { if (!Paused) { UpdateAdvertisementBasedOnCurrentIpAddress(); - //Debug.WriteLine("WiFiAdvertiser, broadcasting advert to: {0}:{1}", _remoteEP.Address, _remoteEP.Port); // TEMPORARY! - _clientBroadcast.BeginSend( + Debug.WriteLine("WM, Work, BeginSend-start"); // TEMPORARY + _clientForBookAdvertSend.BeginSend( _sendBytes, _sendBytes.Length, _remoteEP, SendCallback, - _clientBroadcast + _clientForBookAdvertSend ); + Debug.WriteLine("WM, Work, BeginSend-end"); // TEMPORARY } Thread.Sleep(1000); } @@ -238,14 +225,13 @@ private void Work() catch (SocketException e) { Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); - // Log it. - Debug.WriteLine("WiFiAdvertiser::Work, SocketException: " + e); + EventLog.WriteEntry("Application", "WiFiAdvertiser::Work, SocketException: " + e); // Don't know what _progress.Message() is desired here, add as appropriate. } catch (ThreadAbortException) { _progress.Message(idSuffix: "Stopped", message: "Stopped Advertising."); - _clientBroadcast.Close(); + _clientForBookAdvertSend.Close(); } catch (Exception error) { @@ -265,7 +251,7 @@ public static void SendCallback(IAsyncResult args) { } /// private void UpdateAdvertisementBasedOnCurrentIpAddress() { - _currentIpAddress = _localIp; + _currentIpAddress = _ipForReceivingReplies; if (_cachedIpAddress != _currentIpAddress) { @@ -295,8 +281,10 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() // indicating which of the candidate structs to draw from: WiFi, Ethernet, // or neither. // - private CommTypeToExpect GetInterfaceStackWillUse() + private InterfaceInfo GetInterfaceStackWillUse() { + InterfaceInfo IfaceWifi = new(); + InterfaceInfo IfaceEthernet = new(); int currentIfaceMetric; // Initialize result structs metric field to the highest possible value @@ -310,8 +298,8 @@ private CommTypeToExpect GetInterfaceStackWillUse() if (!allOperationalNetworks.Any()) { - Debug.WriteLine("WiFiAdvertiser, ERROR, no network interfaces are operational"); - return CommTypeToExpect.None; + EventLog.WriteEntry("Application", "WiFiAdvertiser, NO network interfaces are operational"); + return null; } // Get key attributes of active network interfaces. @@ -319,11 +307,7 @@ private CommTypeToExpect GetInterfaceStackWillUse() { // If we can't get IP or IPv4 properties for this interface, skip it. var ipProps = ni.GetIPProperties(); - if (ipProps == null) - { - continue; - } - var ipv4Props = ipProps.GetIPv4Properties(); + var ipv4Props = ipProps?.GetIPv4Properties(); if (ipv4Props == null) { continue; @@ -376,15 +360,15 @@ private CommTypeToExpect GetInterfaceStackWillUse() // - Else there is no winner so return none if (IfaceWifi.Metric < int.MaxValue) { - return CommTypeToExpect.WiFi; + return IfaceWifi; } if (IfaceEthernet.Metric < int.MaxValue) { - return CommTypeToExpect.Ethernet; + return IfaceEthernet; } - Debug.WriteLine("WiFiAdvertiser, ERROR, no suitable network interface found"); - return CommTypeToExpect.None; + EventLog.WriteEntry("Application", "WiFiAdvertiser, NO suitable network interface found"); + return null; } // Get a key piece of info ("metric") from the specified network interface. @@ -420,7 +404,7 @@ int GetMetricForInterface(int interfaceIndex) } catch (OutOfMemoryException e) { - Debug.WriteLine("GetMetricForInterface, ERROR creating buffer: " + e); + EventLog.WriteEntry("Application", "GetMetricForInterface, ERROR creating buffer: " + e); return bestMetric; } @@ -433,7 +417,7 @@ int GetMetricForInterface(int interfaceIndex) // Something went wrong so bail. // It is tempting to add a dealloc call here, but don't. The // dealloc in the 'finally' block *will* be done (I checked). - Debug.WriteLine("GetMetricForInterface, ERROR, GetIpForwardTable() = {0}, returning {1}", error, bestMetric); + EventLog.WriteEntry("Application", $"GetMetricForInterface, ERROR {error} getting table, returning {bestMetric}"); return bestMetric; } @@ -461,7 +445,7 @@ int GetMetricForInterface(int interfaceIndex) { if (e is AccessViolationException || e is MissingMethodException) { - Debug.WriteLine("GetMetricForInterface, ERROR: " + e); + EventLog.WriteEntry("Application", "GetMetricForInterface, ERROR walking table: " + e); } } finally @@ -520,8 +504,7 @@ private string GetDirectedBroadcastAddress(string ipIn, string maskIn) if (ipBytes.Length != maskBytes.Length) { - Console.WriteLine("CalculateBroadcastAddress, ERROR, length mismatch, IP vs mask: {0}, {1}", - ipBytes.Length, maskBytes.Length); + EventLog.WriteEntry("Application", $"BroadcastAddress, ERROR length mismatch, IP vs mask: {ipBytes.Length}, {maskBytes.Length}"); return ""; } @@ -533,9 +516,9 @@ private string GetDirectedBroadcastAddress(string ipIn, string maskIn) return new IPAddress(bcastBytes).ToString(); } - catch (Exception) + catch (Exception e) { - // Invalid IP address or subnet mask. + EventLog.WriteEntry("Application", "BroadcastAddress, Exception: " + e); return ""; } } From 3f8efc8493c7fbd714a4175d645f15818026b082 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Thu, 30 Oct 2025 21:36:15 -0400 Subject: [PATCH 04/10] Restructure advertising loop to include IP address discovery To become aware of IP address changes if the user switches from ethernet to Wi-Fi or vice versa, or some other network interface change, discover local IP before every advert. It adds about 250 msec (on my 1.9 GHz i7 laptop) to the interval between adverts but that's not a problem. Debug output is still present, will be removed at next commit. --- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 138 ++++++++++-------- 1 file changed, 81 insertions(+), 57 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 278efa07bba3..571768806e53 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -40,7 +40,8 @@ public string BookVersion private UdpClient _clientForBookAdvertSend; private Thread _thread; - private IPEndPoint _remoteEP; + private IPEndPoint _bcastDestinationEP; + private IPEndPoint _bcastSourceEP; // Our "local IP address" of the network interface used for Bloom network traffic: // - it is the target address of incoming book requests from Androids @@ -52,7 +53,7 @@ public string BookVersion private string _subnetMask = ""; private string _currentIpAddress = ""; - private string _cachedIpAddress = ""; + private string _previousIpAddress = ""; // Layout of a row in the IPv4 routing table. [StructLayout(LayoutKind.Sequential)] @@ -126,6 +127,63 @@ private void Work() message: "Advertising book to Bloom Readers on local network..." ); + try + { + // Advertise indefinitely. + while (true) + { + if (!Paused) + { + // Determine remote (broadcast) and local IP addresses. This includes + // instantiating the UdpClient, which is why it must be released after + // it does the broadcast. + // This all takes about 250 millisec on a 1.9 GHz Core i7 laptop. + GetCurrentIpAddresses(); + + UpdateAdvertisementBasedOnCurrentIpAddress(); + + Debug.WriteLine("WM, Work, BeginSend-start"); // TEMPORARY + _clientForBookAdvertSend.BeginSend( + _sendBytes, + _sendBytes.Length, + _bcastDestinationEP, + SendCallback, + _clientForBookAdvertSend + ); + Debug.WriteLine("WM, Work, BeginSend-end"); // TEMPORARY + + // Release network resources used by the broadcast. + _clientForBookAdvertSend.Close(); + Debug.WriteLine("WM, Work, UdpClient released"); // TEMPORARY + } + Thread.Sleep(1000); + } + } + catch (SocketException e) + { + Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); + EventLog.WriteEntry("Application", "WiFiAdvertiser::Work, SocketException: " + e); + // Don't know what _progress.Message() is desired here, add as appropriate. + } + catch (ThreadAbortException) + { + _progress.Message(idSuffix: "Stopped", message: "Stopped Advertising."); + _clientForBookAdvertSend.Close(); + } + catch (Exception error) + { + // not worth localizing + _progress.MessageWithoutLocalizing( + $"Error in Advertiser: {error.Message}", + ProgressKind.Error + ); + } + } + + public static void SendCallback(IAsyncResult args) { } + + private void GetCurrentIpAddresses() + { // We must ensure that the local IP address we advertise by the UDP broadcast is the // same address from which the network stack makes the broadcast. Gleaning the local // IP address from a UdpClient usually does yield the one that is being used, but @@ -133,9 +191,8 @@ private void Work() // Android gets the wrong address from the advert, and Desktop won't get the Android // book request. // - // To mitigate, change how the UdpClient is instantiated. Assign it the IP address of - // the network interface the network stack will use: the interface having the lowest - // "interface metric." + // To mitigate, instantiate the UdpClient with the IP address of the network interface + // the network stack will use: the interface having the lowest "interface metric." // // The PC on which this runs likely has both WiFi and Ethernet. Either can work, // but preference is given to WiFi. The reason: although this PC can likely go either @@ -145,7 +202,7 @@ private void Work() // PC and Android are on the same subnet are greatest if both are using WiFi. // Examine the network interfaces and determine which will be used for network traffic. - Debug.WriteLine("WM, Work, getting src and dest IPs"); // TEMPORARY + Debug.WriteLine("WM, GetCurrentIpAddresses, getting src and dest IPs"); // TEMPORARY InterfaceInfo ifcResult = GetInterfaceStackWillUse(); if (ifcResult == null) @@ -166,21 +223,22 @@ private void Work() EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting broadcast address"); return; } - Debug.WriteLine("WM, Work, got src and dest IPs"); // TEMPORARY + Debug.WriteLine("WM, GetCurrentIpAddresses, got src and dest IPs"); // TEMPORARY try { - Debug.WriteLine("WM, Work, try-begin"); // TEMPORARY + Debug.WriteLine("WM, GetCurrentIpAddresses, try-begin"); // TEMPORARY // Instantiate source endpoint using the local IP address we just got, // then use that to create the UdpClient for broadcasting adverts. - IPEndPoint epBroadcast = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); - if (epBroadcast == null) + //IPEndPoint epBroadcast = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); + _bcastSourceEP = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); + if (_bcastSourceEP == null) { - EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating IPEndPoint"); + EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating local IPEndPoint"); return; } - _clientForBookAdvertSend = new UdpClient(epBroadcast); + _clientForBookAdvertSend = new UdpClient(_bcastSourceEP); if (_clientForBookAdvertSend == null) { EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating UdpClient"); @@ -194,68 +252,33 @@ private void Work() _clientForBookAdvertSend.EnableBroadcast = true; // Set up destination endpoint. - _remoteEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); + _bcastDestinationEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); // Log key data for tech support. - EventLog.WriteEntry("Application", $"UDP advertising will use: _localIp = {_ipForReceivingReplies}:{epBroadcast.Port} ({ifaceDesc})"); + EventLog.WriteEntry("Application", $"UDP advertising will use: _localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifaceDesc})"); EventLog.WriteEntry("Application", $" _subnetMask = {_subnetMask}"); - EventLog.WriteEntry("Application", $" _remoteIp = {_remoteEP.Address}:{_remoteEP.Port}"); - Debug.WriteLine("WM, Work, try-end"); // TEMPORARY - - // Advertise once per second, indefinitely. - while (true) - { - if (!Paused) - { - UpdateAdvertisementBasedOnCurrentIpAddress(); - - Debug.WriteLine("WM, Work, BeginSend-start"); // TEMPORARY - _clientForBookAdvertSend.BeginSend( - _sendBytes, - _sendBytes.Length, - _remoteEP, - SendCallback, - _clientForBookAdvertSend - ); - Debug.WriteLine("WM, Work, BeginSend-end"); // TEMPORARY - } - Thread.Sleep(1000); - } + EventLog.WriteEntry("Application", $" _remoteIp = {_bcastDestinationEP.Address}:{_bcastDestinationEP.Port}"); + Debug.WriteLine("WM, GetCurrentIpAddresses, try-end"); // TEMPORARY } - catch (SocketException e) - { - Bloom.Utils.MiscUtils.SuppressUnusedExceptionVarWarning(e); - EventLog.WriteEntry("Application", "WiFiAdvertiser::Work, SocketException: " + e); - // Don't know what _progress.Message() is desired here, add as appropriate. - } - catch (ThreadAbortException) - { - _progress.Message(idSuffix: "Stopped", message: "Stopped Advertising."); - _clientForBookAdvertSend.Close(); - } - catch (Exception error) + catch (Exception e) { - // not worth localizing - _progress.MessageWithoutLocalizing( - $"Error in Advertiser: {error.Message}", - ProgressKind.Error - ); + Debug.WriteLine("WM, GetCurrentIpAddresses, Exception: " + e); // TEMPORARY + EventLog.WriteEntry("Application", "WiFiAdvertiser::GetCurrentIpAddresses, Exception: " + e); } } - public static void SendCallback(IAsyncResult args) { } - /// /// Since this is typically not a real "server", its ipaddress could be assigned dynamically, /// and could change each time someone wakes it up. /// private void UpdateAdvertisementBasedOnCurrentIpAddress() { + Debug.WriteLine("WM, UABOCIA, begin"); // TEMPORARY _currentIpAddress = _ipForReceivingReplies; - if (_cachedIpAddress != _currentIpAddress) + if (_previousIpAddress != _currentIpAddress) { - _cachedIpAddress = _currentIpAddress; + _previousIpAddress = _currentIpAddress; dynamic advertisement = new DynamicJson(); advertisement.title = BookTitle; advertisement.version = BookVersion; @@ -270,6 +293,7 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() _sendBytes = Encoding.UTF8.GetBytes(advertisement.ToString()); //EventLog.WriteEntry("Application", "Serving at http://" + _currentIpAddress + ":" + ChorusHubOptions.MercurialPort, EventLogEntryType.Information); } + Debug.WriteLine("WM, UABOCIA, done"); // TEMPORARY } // Survey the network interfaces and determine which one, if any, the network From 82d6954ef34258c32955ac5befd99bd77523f110 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Fri, 31 Oct 2025 16:39:19 -0400 Subject: [PATCH 05/10] Last debug commit: comments and debug output Improve a few comments. Tweak a bit of debug output. The new advertising loop, in which IP address is discovered before EVERY advert, tests okay. If an advertising session begins with Wi-Fi but midstream changes to Ethernet, WiFiAdvertiser gets it and adjusts. Same thing for the opposite case, Ethernet > Wi-Fi. --- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 571768806e53..74e531a69b8c 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -44,7 +44,7 @@ public string BookVersion private IPEndPoint _bcastSourceEP; // Our "local IP address" of the network interface used for Bloom network traffic: - // - it is the target address of incoming book requests from Androids + // - it is the target address for incoming book requests from Androids // - it is also used to derive the destination address for outgoing book advert broadcasts private string _ipForReceivingReplies = ""; @@ -135,9 +135,13 @@ private void Work() if (!Paused) { // Determine remote (broadcast) and local IP addresses. This includes - // instantiating the UdpClient, which is why it must be released after - // it does the broadcast. - // This all takes about 250 millisec on a 1.9 GHz Core i7 laptop. + // instantiating the UdpClient, which must be released after it does + // the broadcast. + // This all takes about 250 millisec on a 1.9 GHz Core i7 Win11 laptop. + // It does slow the advertising loop by a quarter second, but the user + // experience won't suffer. Even slower machines taking 1000 millisec + // or more to do this will still have adverts going out every 2-3 secs. + // I don't think that would be a big problem. GetCurrentIpAddresses(); UpdateAdvertisementBasedOnCurrentIpAddress(); @@ -230,7 +234,6 @@ private void GetCurrentIpAddresses() Debug.WriteLine("WM, GetCurrentIpAddresses, try-begin"); // TEMPORARY // Instantiate source endpoint using the local IP address we just got, // then use that to create the UdpClient for broadcasting adverts. - //IPEndPoint epBroadcast = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); _bcastSourceEP = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); if (_bcastSourceEP == null) { @@ -255,9 +258,11 @@ private void GetCurrentIpAddresses() _bcastDestinationEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); // Log key data for tech support. - EventLog.WriteEntry("Application", $"UDP advertising will use: _localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifaceDesc})"); - EventLog.WriteEntry("Application", $" _subnetMask = {_subnetMask}"); - EventLog.WriteEntry("Application", $" _remoteIp = {_bcastDestinationEP.Address}:{_bcastDestinationEP.Port}"); + EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifaceDesc})"); + EventLog.WriteEntry("Application", $" subnetMask = {_subnetMask}"); + EventLog.WriteEntry("Application", $" remoteIp = {_bcastDestinationEP.Address}:{_bcastDestinationEP.Port}"); + Debug.WriteLine($" _ipForReceivingReplies = {_ipForReceivingReplies}"); // TEMPORARY + Debug.WriteLine($" _ipForSendingBroadcast = {_ipForSendingBroadcast}"); // TEMPORARY Debug.WriteLine("WM, GetCurrentIpAddresses, try-end"); // TEMPORARY } catch (Exception e) From 93ef15548e2cabbb3533d8d355f8df33f6e94c31 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Mon, 3 Nov 2025 16:34:32 -0500 Subject: [PATCH 06/10] Remove all debug output code This is the last step in making changes per comments in the PR. This commit plus the 3 (I think) previous ones will be pushed to be visible to reviewers. --- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 24 ++++++------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 74e531a69b8c..c864e45fea22 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -146,7 +146,6 @@ private void Work() UpdateAdvertisementBasedOnCurrentIpAddress(); - Debug.WriteLine("WM, Work, BeginSend-start"); // TEMPORARY _clientForBookAdvertSend.BeginSend( _sendBytes, _sendBytes.Length, @@ -154,11 +153,9 @@ private void Work() SendCallback, _clientForBookAdvertSend ); - Debug.WriteLine("WM, Work, BeginSend-end"); // TEMPORARY // Release network resources used by the broadcast. _clientForBookAdvertSend.Close(); - Debug.WriteLine("WM, Work, UdpClient released"); // TEMPORARY } Thread.Sleep(1000); } @@ -189,11 +186,11 @@ public static void SendCallback(IAsyncResult args) { } private void GetCurrentIpAddresses() { // We must ensure that the local IP address we advertise by the UDP broadcast is the - // same address from which the network stack makes the broadcast. Gleaning the local + // actual address from which the network stack makes the broadcast. Gleaning the local // IP address from a UdpClient usually does yield the one that is being used, but // unfortunately it can be different on some machines. When that happens the remote - // Android gets the wrong address from the advert, and Desktop won't get the Android - // book request. + // Android gets the wrong address from the advert, becoming misinformed about where + // to respond. It sends its book request somewhere else, and Desktop never hears it. // // To mitigate, instantiate the UdpClient with the IP address of the network interface // the network stack will use: the interface having the lowest "interface metric." @@ -204,9 +201,11 @@ private void GetCurrentIpAddresses() // and Reader must be on the same subnet. If Desktop is using Ethernet it may not be // on the same subnet as Reader, especially on larger networks. The chances that both // PC and Android are on the same subnet are greatest if both are using WiFi. + // + // This method generates (a) the local IP address *from* which the book advert is + // sent, and (b) the "directed broadcast address" *to* which the book advert is sent. // Examine the network interfaces and determine which will be used for network traffic. - Debug.WriteLine("WM, GetCurrentIpAddresses, getting src and dest IPs"); // TEMPORARY InterfaceInfo ifcResult = GetInterfaceStackWillUse(); if (ifcResult == null) @@ -217,7 +216,6 @@ private void GetCurrentIpAddresses() _ipForReceivingReplies = ifcResult.IpAddr; _subnetMask = ifcResult.NetMask; - string ifaceDesc = ifcResult.Description; // Now that we know the IP address and subnet mask in effect, calculate // the "directed" broadcast address to use. @@ -227,11 +225,9 @@ private void GetCurrentIpAddresses() EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting broadcast address"); return; } - Debug.WriteLine("WM, GetCurrentIpAddresses, got src and dest IPs"); // TEMPORARY try { - Debug.WriteLine("WM, GetCurrentIpAddresses, try-begin"); // TEMPORARY // Instantiate source endpoint using the local IP address we just got, // then use that to create the UdpClient for broadcasting adverts. _bcastSourceEP = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); @@ -258,16 +254,12 @@ private void GetCurrentIpAddresses() _bcastDestinationEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); // Log key data for tech support. - EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifaceDesc})"); + EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifcResult.Description})"); EventLog.WriteEntry("Application", $" subnetMask = {_subnetMask}"); EventLog.WriteEntry("Application", $" remoteIp = {_bcastDestinationEP.Address}:{_bcastDestinationEP.Port}"); - Debug.WriteLine($" _ipForReceivingReplies = {_ipForReceivingReplies}"); // TEMPORARY - Debug.WriteLine($" _ipForSendingBroadcast = {_ipForSendingBroadcast}"); // TEMPORARY - Debug.WriteLine("WM, GetCurrentIpAddresses, try-end"); // TEMPORARY } catch (Exception e) { - Debug.WriteLine("WM, GetCurrentIpAddresses, Exception: " + e); // TEMPORARY EventLog.WriteEntry("Application", "WiFiAdvertiser::GetCurrentIpAddresses, Exception: " + e); } } @@ -278,7 +270,6 @@ private void GetCurrentIpAddresses() /// private void UpdateAdvertisementBasedOnCurrentIpAddress() { - Debug.WriteLine("WM, UABOCIA, begin"); // TEMPORARY _currentIpAddress = _ipForReceivingReplies; if (_previousIpAddress != _currentIpAddress) @@ -298,7 +289,6 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() _sendBytes = Encoding.UTF8.GetBytes(advertisement.ToString()); //EventLog.WriteEntry("Application", "Serving at http://" + _currentIpAddress + ":" + ChorusHubOptions.MercurialPort, EventLogEntryType.Information); } - Debug.WriteLine("WM, UABOCIA, done"); // TEMPORARY } // Survey the network interfaces and determine which one, if any, the network From e5585cc32835e921e08d2b9b3656d1a873f734a2 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Mon, 17 Nov 2025 21:33:32 -0500 Subject: [PATCH 07/10] PR updates, 2nd round: changes to variable names and comments No logic changes. The only substantive changes are variable names per PR review requests. Also improvements to comments that apply to network addressing and descriptions of the mitigation logic. --- .../BloomPub/wifi/BloomReaderUDPListener.cs | 10 +-- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 76 ++++++++++++------- 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs index 520e777eee9d..03d37f03e4f7 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/BloomReaderUDPListener.cs @@ -57,19 +57,19 @@ public void ListenForUDPPackages() // https://learn.microsoft.com/en-us/dotnet/api/system.net.ipaddress.any?view=net-9.0 // Then base the UdpClient on this endpoint. - IPEndPoint groupEP = null; + IPEndPoint epForBookRequestReceive = null; try { - groupEP = new IPEndPoint(IPAddress.Any, _portToListen); + epForBookRequestReceive = new IPEndPoint(IPAddress.Any, _portToListen); - if (groupEP == null) + if (epForBookRequestReceive == null) { EventLog.WriteEntry("Application", "UDPListener, ERROR creating IPEndPoint"); return; } - _clientForBookRequestReceive = new UdpClient(groupEP); + _clientForBookRequestReceive = new UdpClient(epForBookRequestReceive); if (_clientForBookRequestReceive == null) { @@ -96,7 +96,7 @@ public void ListenForUDPPackages() EventLog.WriteEntry("Application", $"UDP listening will wait for packet on {localEP.Address}, port {localEP.Port}"); } - byte[] bytes = _clientForBookRequestReceive.Receive(ref groupEP); // waits for packet from Android. + byte[] bytes = _clientForBookRequestReceive.Receive(ref epForBookRequestReceive); // waits for packet from Android. // DEBUG ONLY //Debug.WriteLine("UDPListener, got {0} bytes (raising \'NewMessageReceived\'):", bytes.Length); diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index c864e45fea22..367f1eb1dab7 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -35,21 +35,36 @@ public string BookVersion } public string TitleLanguage; - // Client by which we send out broadcast book adverts. It uses our local IP address (attached - // to the correct network interface) and the expected port. - private UdpClient _clientForBookAdvertSend; - private Thread _thread; - private IPEndPoint _bcastDestinationEP; - private IPEndPoint _bcastSourceEP; - // Our "local IP address" of the network interface used for Bloom network traffic: - // - it is the target address for incoming book requests from Androids - // - it is also used to derive the destination address for outgoing book advert broadcasts - private string _ipForReceivingReplies = ""; + // Source or "local" IP address of the network interface we determine that the network stack + // will use for Bloom network traffic, both outgoing (adverts) and incoming (book requests): + // - This is the source address for our broadcast advertisements. + // - This is also the destination address for incoming book requests from Androids. It is + // included in the adverts we send out, informing potential Android recipients where to + // send their requests. + // - This is also used to derive the destination address for our broadcast advertisements. + private string _ipForThisDeviceOnChosenInterface = ""; + + // Destination IP address to which outgoing book advertising broadcasts are sent. + // It does not identify a single device but rather all devices on the same subnet. This type + // of address is known as a "directed broadcast address". + private string _ipBroadcastForDestination = ""; + + // Source endpoint used in broadcasting our adverts. + // It is derived from our source IP address. + // It has a hand in configuring the broadcast client used to send adverts, thus ensuring that + // adverts will go out to the correct subnet. + private IPEndPoint _epForThisDeviceOnChosenInterface; + + // Destination endpoint used in broadcasting our adverts. + // It is derived from the destination IP address (i.e., the directed broadcast address). + // The broadcast client used to send adverts takes in this endpoint as a parameter. + private IPEndPoint _epForBroadcastDestination; - // IP address to which outgoing book advert broadcasts are sent, a "directed broadcast address": - private string _ipForSendingBroadcast = ""; + // Client by which we send out broadcast book adverts. It uses our local IP address (attached + // to the correct network interface) and the expected port. + private UdpClient _clientForBookAdvertSend; private string _subnetMask = ""; private string _currentIpAddress = ""; @@ -149,7 +164,7 @@ private void Work() _clientForBookAdvertSend.BeginSend( _sendBytes, _sendBytes.Length, - _bcastDestinationEP, + _epForBroadcastDestination, SendCallback, _clientForBookAdvertSend ); @@ -187,13 +202,18 @@ private void GetCurrentIpAddresses() { // We must ensure that the local IP address we advertise by the UDP broadcast is the // actual address from which the network stack makes the broadcast. Gleaning the local - // IP address from a UdpClient usually does yield the one that is being used, but - // unfortunately it can be different on some machines. When that happens the remote - // Android gets the wrong address from the advert, becoming misinformed about where - // to respond. It sends its book request somewhere else, and Desktop never hears it. + // IP address from an active UdpClient -- using, for example, Dns.GetHostEntry() -- + // usually yields the address that the stack is using. + // But not always. + // On some machines, unfortunately, asking for the IP from the active UdpClient after + // the fact yields the address of an interface NOT being used by the stack. When that + // happens the the wrong address gets put into the advert and the remote Android becomes + // misinformed about where to respond. It sends its book request somewhere else, and + // Desktop never hears it. // // To mitigate, instantiate the UdpClient with the IP address of the network interface - // the network stack will use: the interface having the lowest "interface metric." + // that we are confident the network stack *will* be using: the interface having the + // lowest "interface metric." // // The PC on which this runs likely has both WiFi and Ethernet. Either can work, // but preference is given to WiFi. The reason: although this PC can likely go either @@ -214,13 +234,13 @@ private void GetCurrentIpAddresses() return; } - _ipForReceivingReplies = ifcResult.IpAddr; + _ipForThisDeviceOnChosenInterface = ifcResult.IpAddr; _subnetMask = ifcResult.NetMask; // Now that we know the IP address and subnet mask in effect, calculate // the "directed" broadcast address to use. - _ipForSendingBroadcast = GetDirectedBroadcastAddress(_ipForReceivingReplies, _subnetMask); - if (_ipForSendingBroadcast.Length == 0) + _ipBroadcastForDestination = GetDirectedBroadcastAddress(_ipForThisDeviceOnChosenInterface, _subnetMask); + if (_ipBroadcastForDestination.Length == 0) { EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting broadcast address"); return; @@ -230,14 +250,14 @@ private void GetCurrentIpAddresses() { // Instantiate source endpoint using the local IP address we just got, // then use that to create the UdpClient for broadcasting adverts. - _bcastSourceEP = new IPEndPoint(IPAddress.Parse(_ipForReceivingReplies), _portForBroadcast); - if (_bcastSourceEP == null) + _epForThisDeviceOnChosenInterface = new IPEndPoint(IPAddress.Parse(_ipForThisDeviceOnChosenInterface), _portForBroadcast); + if (_epForThisDeviceOnChosenInterface == null) { EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating local IPEndPoint"); return; } - _clientForBookAdvertSend = new UdpClient(_bcastSourceEP); + _clientForBookAdvertSend = new UdpClient(_epForThisDeviceOnChosenInterface); if (_clientForBookAdvertSend == null) { EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR creating UdpClient"); @@ -251,12 +271,12 @@ private void GetCurrentIpAddresses() _clientForBookAdvertSend.EnableBroadcast = true; // Set up destination endpoint. - _bcastDestinationEP = new IPEndPoint(IPAddress.Parse(_ipForSendingBroadcast), _portForBroadcast); + _epForBroadcastDestination = new IPEndPoint(IPAddress.Parse(_ipBroadcastForDestination), _portForBroadcast); // Log key data for tech support. - EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForReceivingReplies}:{_bcastSourceEP.Port} ({ifcResult.Description})"); + EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForThisDeviceOnChosenInterface}:{_epForThisDeviceOnChosenInterface.Port} ({ifcResult.Description})"); EventLog.WriteEntry("Application", $" subnetMask = {_subnetMask}"); - EventLog.WriteEntry("Application", $" remoteIp = {_bcastDestinationEP.Address}:{_bcastDestinationEP.Port}"); + EventLog.WriteEntry("Application", $" remoteIp = {_epForBroadcastDestination.Address}:{_epForBroadcastDestination.Port}"); } catch (Exception e) { @@ -270,7 +290,7 @@ private void GetCurrentIpAddresses() /// private void UpdateAdvertisementBasedOnCurrentIpAddress() { - _currentIpAddress = _ipForReceivingReplies; + _currentIpAddress = _ipForThisDeviceOnChosenInterface; if (_previousIpAddress != _currentIpAddress) { From 70f1d0e0847a9a996315be0fa8fcb611d0908bbb Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Tue, 18 Nov 2025 11:02:27 -0500 Subject: [PATCH 08/10] Flesh out one key comment a bit more Add words to explicitly explain what cannot be controlled (the network interface chosen by the network stack) and what can (the IP address put into book adverts). --- src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 367f1eb1dab7..8e46458fb201 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -211,9 +211,11 @@ private void GetCurrentIpAddresses() // misinformed about where to respond. It sends its book request somewhere else, and // Desktop never hears it. // - // To mitigate, instantiate the UdpClient with the IP address of the network interface - // that we are confident the network stack *will* be using: the interface having the - // lowest "interface metric." + // The network stack is going to use the interface it wants. We can't control what it + // chooses. But we *can* control the IP address we advertise to remote Androids. We get + // that address up front using the same selection criteria as the network stack: find + // the network interface having the lowest "interface metric" and use *its* IP address. + // We then use that IP address as the basis for the UdpClient. // // The PC on which this runs likely has both WiFi and Ethernet. Either can work, // but preference is given to WiFi. The reason: although this PC can likely go either From f69727a97ce9b95ffc2e041530a7421f254ef62e Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Wed, 26 Nov 2025 12:33:48 -0500 Subject: [PATCH 09/10] Rename a variable, improve a comment Implement 3rd series of responses to PR feedback. Improve a large explanatory comment, and rename one variable for consistency. --- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 8e46458fb201..5828fff942e3 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -49,7 +49,7 @@ public string BookVersion // Destination IP address to which outgoing book advertising broadcasts are sent. // It does not identify a single device but rather all devices on the same subnet. This type // of address is known as a "directed broadcast address". - private string _ipBroadcastForDestination = ""; + private string _ipForBroadcastDestination = ""; // Source endpoint used in broadcasting our adverts. // It is derived from our source IP address. @@ -214,8 +214,9 @@ private void GetCurrentIpAddresses() // The network stack is going to use the interface it wants. We can't control what it // chooses. But we *can* control the IP address we advertise to remote Androids. We get // that address up front using the same selection criteria as the network stack: find - // the network interface having the lowest "interface metric" and use *its* IP address. - // We then use that IP address as the basis for the UdpClient. + // the network interface having the lowest "interface metric" -- Ethernet or WiFi, it + // could be either -- and advertise *that* interface's IP address. + // We then use that same IP address as the basis for the UdpClient. // // The PC on which this runs likely has both WiFi and Ethernet. Either can work, // but preference is given to WiFi. The reason: although this PC can likely go either @@ -241,8 +242,8 @@ private void GetCurrentIpAddresses() // Now that we know the IP address and subnet mask in effect, calculate // the "directed" broadcast address to use. - _ipBroadcastForDestination = GetDirectedBroadcastAddress(_ipForThisDeviceOnChosenInterface, _subnetMask); - if (_ipBroadcastForDestination.Length == 0) + _ipForBroadcastDestination = GetDirectedBroadcastAddress(_ipForThisDeviceOnChosenInterface, _subnetMask); + if (_ipForBroadcastDestination.Length == 0) { EventLog.WriteEntry("Application", "WiFiAdvertiser, ERROR getting broadcast address"); return; @@ -273,7 +274,7 @@ private void GetCurrentIpAddresses() _clientForBookAdvertSend.EnableBroadcast = true; // Set up destination endpoint. - _epForBroadcastDestination = new IPEndPoint(IPAddress.Parse(_ipBroadcastForDestination), _portForBroadcast); + _epForBroadcastDestination = new IPEndPoint(IPAddress.Parse(_ipForBroadcastDestination), _portForBroadcast); // Log key data for tech support. EventLog.WriteEntry("Application", $"UDP advertising will use: localIp = {_ipForThisDeviceOnChosenInterface}:{_epForThisDeviceOnChosenInterface.Port} ({ifcResult.Description})"); @@ -512,7 +513,7 @@ int GetMetricForInterface(int interfaceIndex) // subnet mask, both of which are passed in to this function. // // Bit-wise rationale: - // The subnet mask indicates how to handle the IP address bits. + // The subnet mask indicates how to handle the IP address bits. // - '1' mask bits: the "network address" portion of the IP address. // The broadcast address aims at the same network so just copy the IP // address bits into the same positions of the broadcast address. From cf49d4a6b819f85bff4596059f9081a8f19d8b14 Mon Sep 17 00:00:00 2001 From: Wade Mergenthal Date: Thu, 4 Dec 2025 15:58:50 -0500 Subject: [PATCH 10/10] Rename GetInterfaceStackWillUse() and modify associated comments GetInterfaceStackWillUse() is not modified but it is renamed, to GetInterfaceToAdvertise(). Associated comments did not describe it accurately so they are now fixed. --- .../Publish/BloomPub/wifi/WiFiAdvertiser.cs | 53 ++++++++++++------- 1 file changed, 35 insertions(+), 18 deletions(-) diff --git a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs index 5828fff942e3..1e4bdd0c3afa 100644 --- a/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs +++ b/src/BloomExe/Publish/BloomPub/wifi/WiFiAdvertiser.cs @@ -213,23 +213,38 @@ private void GetCurrentIpAddresses() // // The network stack is going to use the interface it wants. We can't control what it // chooses. But we *can* control the IP address we advertise to remote Androids. We get - // that address up front using the same selection criteria as the network stack: find - // the network interface having the lowest "interface metric" -- Ethernet or WiFi, it - // could be either -- and advertise *that* interface's IP address. - // We then use that same IP address as the basis for the UdpClient. + // that address up front via an educated guess of which network interface the stack will + // choose, and advertise that interface's IP address. // - // The PC on which this runs likely has both WiFi and Ethernet. Either can work, - // but preference is given to WiFi. The reason: although this PC can likely go either - // way, the Android device only has WiFi. For the book transfer to work both Desktop - // and Reader must be on the same subnet. If Desktop is using Ethernet it may not be - // on the same subnet as Reader, especially on larger networks. The chances that both - // PC and Android are on the same subnet are greatest if both are using WiFi. + // This educated guess starts by checking each network in the system and, if it is up, + // noting its "interface metric" -- the main criterion the stack uses most of the time. + // There are rare exceptions when additional criteria carry greater weight, resulting + // in the stack making a different selection. As far as I can tell, those are rare + // enough and have mitigations complex enough that accounting for them could introduce + // more risk than reward. For more information these posts are helpful: + // https://learn.microsoft.com/en-us/troubleshoot/windows-server/networking/automatic-metric-for-ipv4-routes + // https://en.softcomputers.org/change-wireless-network-connection-priority-in-windows/ + // + // The stack normally chooses the network interface having the lowest interface metric. + // This could be either Ethernet or WiFi (most PC laptops have both). However, we will + // prefer WiFi since (a) the Android only has WiFi, and (b) a successful book transfer + // requires both Desktop and Reader to be on the same subnet. If Desktop is using Ethernet + // it may not be on the same subnet as Reader, especially on larger networks. The chances + // that both PC and Android are on the same subnet are greatest if both are using WiFi. + // + // Our process boils down to this: + // 1. Find the active WiFi interface with the lowest interface metric, + // and use its IP address; + // 2. Otherwise, find the active Ethernet interface with the lowest interface metric, + // and use its IP address; + // 3. Otherwise, fail: IP address not found. + // The winning IP address is then used as the basis for the UdpClient which performs the + // actual data transfer. // // This method generates (a) the local IP address *from* which the book advert is // sent, and (b) the "directed broadcast address" *to* which the book advert is sent. - // Examine the network interfaces and determine which will be used for network traffic. - InterfaceInfo ifcResult = GetInterfaceStackWillUse(); + InterfaceInfo ifcResult = GetInterfaceToAdvertise(); if (ifcResult == null) { @@ -307,6 +322,8 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() // If we do eventually add capability to display the advert as a QR code, // the local IP address will need to be included. Might as well add it now. + // It will be visible in a network trace, showing what Desktop considers + // its local IP to be. advertisement.senderIP = _currentIpAddress; _sendBytes = Encoding.UTF8.GetBytes(advertisement.ToString()); @@ -314,16 +331,16 @@ private void UpdateAdvertisementBasedOnCurrentIpAddress() } } - // Survey the network interfaces and determine which one, if any, the network - // stack will use for network traffic. + // Survey the network interfaces and make an educated guess as to which one, + // if any, the network stack will use for network traffic. // - During the assessment the current leading WiFi candidate will be held in // 'IfaceWifi', and similarly the current best candidate for Ethernet will // be in 'IfaceEthernet'. - // - After assessment inform calling code of the winner by returning an enum - // indicating which of the candidate structs to draw from: WiFi, Ethernet, - // or neither. + // - After assessment inform calling code of the winner by returning the winning + // candidate struct to draw from -- WiFi or Ethernet -- or null if there is + // no winner. // - private InterfaceInfo GetInterfaceStackWillUse() + private InterfaceInfo GetInterfaceToAdvertise() { InterfaceInfo IfaceWifi = new(); InterfaceInfo IfaceEthernet = new();