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

Protect the user from common connnection configuration mistakes #3700

Open
nohal opened this issue Feb 29, 2024 · 64 comments
Open

Protect the user from common connnection configuration mistakes #3700

nohal opened this issue Feb 29, 2024 · 64 comments

Comments

@nohal
Copy link
Collaborator

nohal commented Feb 29, 2024

It should be pretty easy to protect the users from common nonsense in the configuration, for example TCP and Signal K endpoints must not have broadcast or multicast addresses like 0.0.0.0

Also the default ports can be adjusted based on the protocol (eg. 10110 for NMEA0183, 3000 for Signal K etc.)

@nohal nohal added this to the OpenCPN 5.10.0 milestone Feb 29, 2024
@nohal
Copy link
Collaborator Author

nohal commented Mar 29, 2024

UDP output connection should in turn always use a broadcast or mutlicast address

@transmitterdan
Copy link
Collaborator

A UDP output connection might be good to have the "suggested" IP address be a broadcast/multicast address. But I think it's ok to let the user change it to any IP address. There are valid use cases for sending UDP to a specific device and not all devices (e.g autopilot, data recorder, etc.).

@nohal
Copy link
Collaborator Author

nohal commented Mar 29, 2024

Yes, even 192.168.1.4.x.x for which the shit is flying at us now is actually a completely valid target address, just not an IP address but a hostname...

@transmitterdan
Copy link
Collaborator

Ugh...

@nohal
Copy link
Collaborator Author

nohal commented Mar 29, 2024

We should probably not forbid "anything", but show some warning when it is "suspicious".

@Hakansv
Copy link
Contributor

Hakansv commented Mar 29, 2024

"but a hostname.."

Ahaa.. That's why it was sometimes accepted! Didn't noticed first at the blurry screenshot.

Agree on discussion above. An output (UDP)address not in the form xxx.yyy.zzz.255 would create an advisory message?

@nohal
Copy link
Collaborator Author

nohal commented Mar 29, 2024

For UDP output, if I was to decide, any address not being a unicast (0.0.0.0) broadcast (usually *.*.*.255 in the super simple networks the users have, but it is not this simple) or multicast (Most significant part binary 1110) address should probably result in a warning.
Of course a single IP or a hostname is absolutely correct UDP target, but in our case it is usually an error caused by the user having absolutely no idea what he is doing...

@Hakansv
Copy link
Contributor

Hakansv commented Mar 29, 2024

If all users know what he/she is doing all time we don't need any advisory messages or default values? :)

@rgleason
Copy link
Collaborator

rgleason commented Mar 29, 2024

Is there a way to have a "hostname" advisory if OpenCPN is going to use it that way?
This is a very important discussion, in my opinion. We need to find a way for New Users to get connected to their gear.
Franciska struggle and rescue outline in Connections > Advanced and in CF Thread Still trouble running NMEA data stream and Radar in parallel. Any help?

@rgleason
Copy link
Collaborator

rgleason commented Mar 29, 2024

  • If you select Receive Input on this Port then the connection will listen for another device on the network. This is the default UDP connection setting.
  • For UDP input no IP is required. The default Address entry of 0.0.0.0 will usually be correct. You will need to enter the DataPort that is used by the sender.

Please verify that this is the case.
There is a difference between "no IP is required" and "the address field is ignored".
Is the Address field used for a UDP input connection? <---- Can someone answer this question?

or is it just ignored? Maybe it should be the default 0.0.0.0 greyed? and then the user has to enter they Data Port:
Could we put in some nutty Address like 168.7.182.55 and it would be ignored and still work? Then the user thinks when they are using UDP that they have an Address of 168.7.18.2.55 and that it works.
Are there some guardrails for users here?

the receive input(only), both and output (only) on this port default settings appear to be
Port Address: 0.0.0.0
Data Address: 10110
These can all be changed and are not greyed out in any instance.

@nohal
Copy link
Collaborator Author

nohal commented Mar 29, 2024

Of course it is.

@TwoCanPlugIn
Copy link
Contributor

Yes, even 192.168.1.4.x.x for which the shit is flying at us now is actually a completely valid target address, just not an IP address but a hostname..

Since when did .x become a valid top level domain name ?

And does whatever code that parses the IP address perform a gethostbyname call ?

UDP output connection should in turn always use a broadcast or mutlicast address

Huh, What if I just want to send a UDP output to a single device on the network ?

For UDP input no IP is required. The default Address entry of 0.0.0.0 will usually be correct. You will need to enter the DataPort that is used by the sender.

What happens if the user has multiple network adapters, connected to different subnets ? Perhaps they just want to receive UDP traffic from a single adapter connected to one network.

@nohal
Copy link
Collaborator Author

nohal commented Mar 30, 2024

Since when did .x become a valid top level domain name ?

Since always, it is not an official TLD, but technically valid in a private network? Of course yes.

Huh, What if I just want to send a UDP output to a single device on the network ?

Then what you want to do is a different thing than the usual use case of our users who want to broadcast to their phones and laptops and fill in a single IP address only because they have no idea what they are doing and that there is a difference between UDP broadcast and TCP server. Do you have to be able to do it? Of course.

@TwoCanPlugIn
Copy link
Contributor

TwoCanPlugIn commented Mar 30, 2024

If the user has configured something.x as a hostname, even though .x is not a valid TLD, if they have not added something.x to their hosts file or dns server, it won't resolve, which you can detect by using gethostbyname (superceded on Linux by getaddrinfo),

For output, why not something similar to the SignalK UI ? And you can programmatically determine and pre-populate the subnet broadcast address
sk-udp2

For fuck's sake. Just tested this, As McEnroe would say, You cannot be serious.

forfucksake

@nohal
Copy link
Collaborator Author

nohal commented Mar 30, 2024

Yes, these are exactly the types of things I had on mind when I created this issue.

@B3Free
Copy link

B3Free commented Mar 31, 2024

I've been doing some testing on the UDP connections settings. Whatever address you put in the Address field for the output connection is used as the destination address. If you put a host address in then it will send it to the host. If you put a broadcast address it will send it to the broadcast address.

One "odd" behavior I noticed was that while the debug window would gladly claim to send the UDP packet regardless of the destination that did not always translate to a packet actually getting on the wire. If I used a destination address of a device that was active on the network then it faithfully put the packet on the wire. If I used a destination address of a device that was turned off the debug window would say it was sent but nothing was actually transmitted.

I'm testing on a Windows machine and all of the addresses were on the local subnet. I suspect that something in the network stack decided that if it could not determine the MAC address for the target it was not going to bother putting it on the network. I only mention it because the debug window implied that the message had been sent. I did not think to test it but it would make sense that if I had used an address on a different network (good or bad) then it would have forwarded it to the gateway router without any issue.

I tried to duplicate the odd behavior I saw with Franzisca where her phones would see the UDP packets even though the entry in the data field was not a valid IP address and was (at best) a very odd host address. Thanks nohal, It did not occur to me that it could be seen that way.

When I entered the same odd address I did not see any data on the network. The debug window faithfully showed that it was sending a packet to the odd address but I did not get the same results she did. Something on her system seems to have resolved the address to the local network broadcast address. I suppose a "helpful" consumer-grade DNS might decide that anything it does not know how to resolve should be thrown out to the broadcast address of the local network in the hopes that something on the local network was supposed to use it.

The input address field works in a similar way. If you put 0.0.0.0 in then it will use data from any device as long as the port number matches. If you put a host address in then it will only process data from the indicated device and port number combination. I did not identify any anomalies in the debug window when testing input connections.

@nohal
Copy link
Collaborator Author

nohal commented Mar 31, 2024

We have no problem with valid addresses, it works for all the cases - unicast, broadcast and multicast.

UDP is a connectionless protocol, so the application basically is throwing packets down the drain and by design there is no feedback - as long as we were able to hand over the packet to the OS, it is a success for us, but there never is any warranty or feedback on delivery.

At the time of configuration, we can check that a unicast or broadcast address is directly routeable using the current routing table (which very likely is what the user is trying to do), but there are exceptions like forwarding AIS feed to AISHub, which is a unicast to a public IP somewhere on the internet, which may or may not be available at all and is routed via default gateway.
Anything not being routeable at the moment of configuration, even when being a private local address, does not mean it is wrong, for example most of the boats I work with routinely switch between at least two wi-fi networks with different addressing providing and consuming data (Usually one from the MFD and one from the Cortex or some multiplexer - which one depends on what is turned on/broken at the time or where on the boat you are, not on the user willing to switch).

For multicast, there is nothing to check, that simply is a hole to throw packets to and forget about them.

An interesting subject is total garbage of an input. Two options there, either reject or at least warn if not resolvable and routeable and at the end probably just generate "192.168.1.4.x.x was in the french manual and does not work" as a result. Or let it end up a broadcast like now.

@rgleason
Copy link
Collaborator

rgleason commented Mar 31, 2024

Warren [wholybee] has some suggestions here, for clearer operation:
https://www.cruisersforum.com/forums/f134/still-trouble-running-nmea-data-stream-and-radar-in-parallel-any-help-284739.html#post3885974

Drawing from my experience with UDP unicast, multicast, and broadcast with other products, maybe there could be an UI change on the connections dialog that will prevent common errors. The following is commonly used with professional video equipment I have used.

What I have seen before:
A radio button to select between Unicast, Multicast, and Broadcast. There is a brief definition of each (transmit to one device, several devices, or the whole network). The UI will then change to reflect different options, while the networking code is unchanged.

If the receive button is checked:
If the Unicast button is selected:
IP address field is removed, 0.0.0.0 default is assumed. (suggest that 0.0.0.0 be shown greyed out, cannot be changed)
If the Multicast button is selected:

IP address is populated with a sensible default, such as 224.0.0.78 (78 is just a number I made up, we can choose anything for the 4th octet as our default) A note is displayed to explain that the multicast address is NOT the IP address of the other machine, and it can be left as default unless the user is advanced and knows why it is being changed. (conflicts with some other multicast device-highly unlikely on a boat.

If the broadcast button is displayed:
IP address is hidden, and a default value of 0.0.0.0 is used. (0.0.0.0 shown greyed)

If the transmit button is selected:
If the unicast button is selected:
IP address field is available, and blank. A note states it should be the IP address of the receiving device.
If the multicast button is selected:
IP address is populated with a sensible default, such as 224.0.0.78 (78 is just a number I made up, we can choose anything here as our default) A note is displayed to explain that the multicast address is NOT the IP address of the other machine, and it can be left as default unless the user is advanced and knows why it is being changed. (conflicts with some other multicast device-highly unlikely on a boat

If broadcast is selected:
The IP address is populated with the correct value, by querying the OS for the machines IP address and netmask. A note will state that this value is probably correct, but might need adjusted for advanced configurations with multiple network interfaces.

Selecting both transmit and receive on UDP should be prohibited, and TCP should be advised. While I believe it is technically possible, it introduces complications, what happens when multiple devices are transmitting on the same broadcast or multicast? Maybe it could be allowed with UDP/Unicast, but mostly I think that for bidirectional communication TCP should be preferred. Maybe an "I know what I am doing" checkbox to allow it.

Also, FWIW, previously I have been unable to get Multicast to work at all with OCPN. It might have been an error in my setup at the time, since I easily got broadcast to work I didn't pursue it further. But, someone should confirm that it actually works if it is going to be documented.


-Warren

Perhaps Bill [Be Free} has some thoughts about this?

@B3Free
Copy link

B3Free commented Mar 31, 2024

If the receive button is checked:
If the Unicast button is selected:
IP address field is removed, 0.0.0.0 default is assumed. (suggest that 0.0.0.0 be shown greyed out, cannot be changed)

Unicast from a receive standpoint should have the address of the transmitting device. "I only want data from this address on the specified port. That is actually the way the dialog works now.

An address of 0.0.0.0 on a receive connection means "I want data from any device on the specified port". That is covered by your third option.

I would even go a little further and give them the choice of "accept data from a specific device, accept data from a group of devices, or accept data from any device on my network" radio buttons (or something similar). Unicast, Multicast, and Broadcast are not terms the average user likely cares about. That should cover all of the easy choices.

If you don't lock out the default multicast and broadcast addresses a professional would know that these fields could be changed. Of course, the appropriate manual would use the appropriate terms and specify that any desired multicast or broadcast address may be entered.

Other than mentioning that this may require additional configuration on their router, I would not go any further. That is beyond the scope of an OpenCPN manual. The topic at hand is protecting a normal user from common configuration mistakes.

I am not a normal user and recognize that fact. I have multiple networks on my boat and some of my instruments communicate with devices outside my boat. I designed it, configured it, and it works. As long as my tools, OpenCPN included, allow me to configure them to fit my self-imposed complexities I am content.

@B3Free
Copy link

B3Free commented Mar 31, 2024

Warren,
I was also unable to get multicast to work. The debug window indicated that it was sending data to the multicast address but I never saw any data on the wire.

Honestly, even allowing for my penchant for designing complex networks just to see if they will work, I can't think of a reason I'd set up a multicast connection.

The UI really does not need to change. Any address we guess will be just that (a guess). It's only a few more keystrokes for the user to enter the entire multicast address either because they are following instructions from someone who set up the other end of the connection or because they designed it and know what address to use.

@B3Free
Copy link

B3Free commented Mar 31, 2024

Pavel,
I will admit ignorance as to how the "garbage" address turned into a broadcast address. It does not do that on my network or with my copy of OpenCPN. My money is still on an "overly helpful" DNS server.

How hard would it be to add a ping to any unicast address entered? That would cover routeable and resolvable in one test. If it fails tell the user something like "The host can't be reached at this time." Even if it resolved to the broadcast address, that would be an error in this case since the user said they wanted unicast. It's been a long time since I've seen a ping to a broadcast address return a positive result (by default) and I've never seen a consumer-grade router that could be configured to allow it.

@wholybee
Copy link
Contributor

Bill,
I have to disagree. The interface needs to change.

If the default should not be 0.0.0.0 but something else that is fine. The point is that if Unicast is selected, the user should not be able to enter anything in there. The correct value would populate. My understanding is that on a receiver, the IP address is the address listened to, not transmitted from. So, either the local interface's address(or even 127.0.0.1) , the networks broadcast address, or a chosen multicast address. In a typical Unicast setup, only a port number is needed. In other unicast applications I have never needed to enter anything other than a port number to listen to. Entering the address of the transmitting device should not work, because the transmitting device is not transmitting to that destination.

The reason for Multicast, is that in general, and certainly on any network more complex than a boat, broadcast should not be used. Broadcast floods the network with packets that go to every device on the network. Those devices can choose to do something with them or not. Multicast packets are based on a subscription. Only devices that subscribe get the packets, so much less traffic is generated. Multicast packets are also routable, so in a network with multiple subnets, or crossing a gateway to another location, multicast packets can be routed to it. Probably no boat does this, but it is possible with Multicast.

@B3Free
Copy link

B3Free commented Mar 31, 2024

I'm with you 100% on the receiver as far as the address that should be entered for the receiver is the address that will be listened to. That is usually going to be the address of another device on the network although there is nothing stopping you from listening to your own address if you really want to.

I may be misunderstanding you but it sounds like what you are describing is putting the address of the interface being listened to rather than the address of the host being listened to.

"Broadcast receiver" is really an oxymoron. What we are calling a "broadcast receiver" would be more properly described as an "indiscriminate receiver." A broadcast receiver is a little odd in that you enter 0.0.0.0 which is more accurately described as "any host" rather than the 255.255.255.255 or X.X.X.255 (among many others) which could be better described as "every host" and would be used on a broadcast sender.

@B3Free
Copy link

B3Free commented Mar 31, 2024

Now, assuming that I'm not totally mistaken with a unicast receiver the user is the only one who knows what should be entered. The only thing I know for sure is that 0.0.0.0 is not the right answer since it means "receive from any host" which is the exact opposite of unicast.

My tests yesterday indicated that 0.0.0.0 would accept data from any sender on the specified port but when I put a specific host in the address field that was the only host that the receiver would accept data from.

@B3Free
Copy link

B3Free commented Mar 31, 2024

The transmitting device does not need to know (but may know) the address of the receiving device. The sending device could be sending directly to the receiving device (unicast send), or to any broadcast address that the receiving device may be listening to (broadcast send). (I'm not commenting on multicast since I've never used it and could not make it work in my tests.)

The unicast receiver does not need to know ahead of time if the sender is using unicast or broadcast. Either one will come up the stack to the application and will be accepted if it came from the indicated host.

@wholybee
Copy link
Contributor

wholybee commented Apr 1, 2024

One of us may be backwards about the receive side. I am working from my experience with professional equipment a using UDP unicast and multicast and as I said broadcast shouldn't really be used, so other than OpenCPN I have not used broadcast. My understanding is that actually all 3 work exactly the same, only that for unicast and broadcast the interface by default will listen to it's own IP address and it's own broadcast address, so only a port is needed.

Excuse my poor attempt at making a table of my understanding.

Transmitter: IP 192.168.1.1                 Receiver: IP 192.168.1.2 (by default the network 
                                          interface will listen to 192.168.1.2 and 192.168.1.255
                                          regardless of what the application does)
----------------------------------------------------------------------------------------------------
Transmit to 192.168.1.2                      Is by default always listening on 192.168.1.2 so works
Transmit to 192.168.1.2                      will also work if listening to 127.0.0.1
Transmit to 192.168.1.255                  Is by default always listening on 192.168.1.255 so works
Transmit to 224.0.0.1                          Will work *if* subscribed and listening to 224.0.0.1
                                          The application needs to tell the interface to listen to that address, 
                                          and the operating system/network driver will recognize it
                                          as a multicast address and send the subscription request.
                                          Importantly, it needs to be a valid multicast address, or
                                          the OS/network driver will not send the subscription.
Transmit to 192.168.1.123                 If set to listen to 192.168.1.123 will NOT work. Because
                                          This is not a multicast address, no subscription is sent
                                          and the traffic will be blocked at the network switch.
Transmit to 192.168.1.2                    Even if incorrect IP address is entered, will still work 
                                          because interface always listens to it's own address.
Transmit to 192.168.1.255                Again, any incorrect address can be entered, because the
                                           interface is always listening to its own broadcast
                                           address.

The transmit side is always the IP address you are transmitting to. The receive side is the IP address of the interface the receive device is listening on. Usually there is only one interface installed in a machine. In the video applications I have installed there are often 4 or 6. So it does matter that it is correct. 127.0.0.1 would listen on all 4 or 6 of them. In a very advanced configuration you might specify that ETH1 should listen on 224.0.0.1, so that the multicast stream isn't received on multiple interfaces. That actually IS an option on some of the equipment I have worked with. Specifying multicast addresses is done per interface. Unicast or broadcast doesn't need an IP address at all, only a port number and an interface to listen on.

Sometime tomorrow I can setup some tests and test all of those scenarios. But that is my understanding from setting up a fair number of multicast networks.

@B3Free
Copy link

B3Free commented Apr 1, 2024

Ok, I think I see where the differences are coming from. Let me know if this sounds right.

What you are describing is absolutely correct if you are talking is what is happening at layer 5 of the OSI model. Multicast actually happens at layer 2 and is in some cases indistinguishable from broadcast.

Any packet addressed to the receiver's address or to any broadcast address that the receiver is part of will be passed up the stack to the application layer where OpenCPN resides. What happens at this point is what we are discussing.

Transmitter: IP 192.168.1.1 Receiver: IP 192.168.1.2 (by default the network
interface will listen to 192.168.1.2 and 192.168.1.255
regardless of what the application does)

Transmit to 192.168.1.2 Is by default always listening on 192.168.1.2 so works
Transmit to 192.168.1.2 will also work if listening to 127.0.0.1
Transmit to 192.168.1.255 Is by default always listening on 192.168.1.255 so works

I think at this point when you say "listening" that you are describing the behavior of OpenCPN, not the network card. Please forgive me if I'm wrong about that. If I'm right, this is the way I'd expand it. I don't think the loopback address adds anything to the example so I'm going to drop it.

Transmit to 192.168.1.2 Is by default always listening on 192.168.1.2. Passes up the stack to OpenCPN.
Unicast listener configured for 192.168.1.1 works. All other addresses ignored.
Broadcast (indiscriminate) listener configured for 0.0.0.0 works regardless of source or
destination.
Transmit to 192.168.1.255 Is by default always listening on 192.168.1.255. Passes up the stack to OpenCPN.
Unicast listener configured for 192.168.1.1 works. All other addresses ignored.
Broadcast (indiscriminate) listener configured for 0.0.0.0 works regardless of source or
destination.
Transmit to 192.168.1.123 Neither the network card nor OpenCPN will ever see this packet under normal
circumstances. I can think of several situations why it would not and at least two
where it could but even if the network card receives it the packet will not be sent up
to the application layer (absent a promiscuous mode driver).

Transmit to 192.168.1.2 Even if incorrect IP address is entered, will still work. I agree at level 5. The network driver
running at level 5 does not know what OpenCPN thinks at layer 7. However, when it
passes the packet up to OpenCPN it will not be from the "incorrect IP address" so it will
be ignored.
Transmit to 192.168.1.255 Again, any incorrect address can be entered, because the interface is always listening to its
own broadcast. Again, I agree at level 5 and suggest that OpenCPN will respond the same
as the previous example.

The transmit side is always the IP address you are transmitting to. The receive side is the IP address of the interface the receive device is listening on.

We are almost in agreement. The address on the transmit connection in OpenCPN is always the IP address you are transmitting to. However, the address on the receive connection is either the address of the sender (unicast receiver) or 0.0.0.0 for what I'm calling the "indiscriminate" listener.

I look forward to the results of your test.

@wholybee
Copy link
Contributor

wholybee commented Apr 1, 2024 via email

@wholybee
Copy link
Contributor

wholybee commented Apr 4, 2024 via email

@bdbcat
Copy link
Member

bdbcat commented Apr 4, 2024

Whew...
Even with a clear explanation like the above, the head spins with possibilities. No way is a user going to be able to grok this from a simple config dialog.

Lets consider this possible UI, for UDP only.

  1. If UDP option is selected, the IP address is not presented by default.
  2. Add a checkbox: "Multicast", defaults off. 99% of users may safely ignore. If checked ON, show and enable address field.
  3. if UDP TX is selected, RX checkbox is disabled, and address field is available and required.

Everything after (1) is Advanced fu. We may (safely) assume that a user needing this functionality will a) know what they are doing, or b) know how to ask for help, or c) RTFM.

Did I miss anything?

@wholybee
Copy link
Contributor

wholybee commented Apr 4, 2024 via email

@TwoCanPlugIn
Copy link
Contributor

Some mockups for UDP Receive.

The only time a user enters an address is if they want to listen to a multicast, in which case the "experienced" user should know what they are doing. Nonetheless, we should validate the address as a valid IPv4 address in the correct IP address range.

Haven't got around to thinking about transmit, nor about the elephant in the room, IPv6.

net0 net1 net2 net3 net4

@wholybee
Copy link
Contributor

wholybee commented Apr 4, 2024 via email

@TwoCanPlugIn
Copy link
Contributor

Surprised no one has complained that I had omitted the Loopback interface!

For receive, other than probably correctly implementing the multicast receiver, I don't think it affects the underlying network code.

  1. All - Code binds to INADDR_ANY. All network interfaces receive Unicast, Multicast and Broadcast traffic. This is what happens today if the user enters 0.0.0.0
  2. Single Interface - Code binds to the IP Address. The interface receives Unicast, Multicast and Broadcast traffic.
  3. Unicast, Code binds to INADDR_ANY, and sets the socket option IP_ADD_MEMBERSHIP for the Multicast address. It should only receive multicast traffic. Should verify that it is a valid IP address and within the correct range of multicast addresses.

For transmit, it shouldn't be much different, other than deciding whether Broadcast and Multicast are sent over all interfaces, or whether to specify a single interface.

I think for broadcast it would mean iterating over all interfaces and sending to each broadcast address. and for multicast, IIRC, it would require the socket option IP_MULTICAST_IF. In either case I doubt it would be the intent of the user to send broadcast or multicast traffic over all of their networks.

Alternatively, assume a simple approach for the majority of use cases, and just use the Broadcast Address for the selected interface. There could be an "Advanced Mode" that allows the selection and input of either a single or multicast address.

Importantly, and this was the crux of the matter from the OP, whatever IP address is entered, it needs to be validated.

@nohal
Copy link
Collaborator Author

nohal commented Apr 5, 2024

The matter for the OP was not a problem with address, it was that the interface was being turned off by Windows. The nonsense address resulted in a broadcast.

You can't receive multicast on INADDR_ANY, that is the whole point of it.

The mockup is fine unless I want to connect to something over the internet or need to configure something that is not currently connected, which both is something I use daily both in the lab and on boats.

There is no magic in the network configuration, the user needs to be offered local interface addresses, corresponding broadcast addresses for local interfaces, IPs of the gateways for the local interfaces (Which likely is a multiplexer, RPi with openplotter or a MFD and may provide TCP data), and 0.0.0.0, filtered per scenario (=TCP/UDP/in/out):

  • 0.0.0.0 makes no sense for UDP broadcast transmission
  • Broadcast addresses make no sense for UDP reception
  • Broadcast addresses make no sense for TCP under any circumstances
  • 0.0.0.0 makes no sense for TCP client
  • Perhaps some more, but the above are the most important

Multicast is not something that needs any support other than allowing manual entry of the address as it is only used by people who know what they are doing

@wholybee
Copy link
Contributor

wholybee commented Apr 5, 2024 via email

@wholybee
Copy link
Contributor

wholybee commented Apr 5, 2024 via email

@rgleason
Copy link
Collaborator

rgleason commented Apr 5, 2024

Can someone please summarize the general concensus for this topic?

@TwoCanPlugIn
Copy link
Contributor

You can't receive multicast on INADDR_ANY, that is the whole point of it.

Yes you are correct. I shouldn't put pen to paper when sitting jetlagged in an airport lounge. In my reply above, point 1. should have omitted Multicast and just said Broadcast and Unicast, and Point 3 was mislabelled and, should have been labelled Multicast.

Anyway, I don't know why we're having this discussion, seems to me that everything works without any input validation. My script is just sending traffic to the network broadcast address (192.168.1.255), so apparently a misconfigured input or output port defaults to the broadcast address, which is probably what the end user is trying to achieve.

The good news is that on O5.9, illegal characters cause a crash.

forfucksake-02

@rgleason
Copy link
Collaborator

rgleason commented Apr 5, 2024

@TwoCanPlugIn So your conclusion is change nothing for v5.9? It seems to me that the initial interface could present a very simple form that is good for 75% of the users, showing default settings that do not change in grey, and then allow increasingly atypical configurations with use of dropdown menus which open up and change additional settings. Similar to what was being discussed.

@TwoCanPlugIn
Copy link
Contributor

TwoCanPlugIn commented Apr 6, 2024

Sorry Rick, seems like you missed the gist of my comment.

I think a bit more thought needs to go into this. Pretty soon every boat will have a network of some description, not just NMEA 0183/2000 gateways, but also connections to tablets at the helm, Internet connections using Cellular Mobile or Starlink etc. and O needs to get this right.

Off the top of my head.

  1. the lack of input validation.
  2. currently nonsensical addresses (especially on output) result in broadcasts. Who knows if that was the user's intent.
  3. seems that multicast is lacking a few things: For input, it doesn't use SO_REUSEPORT which means another application running on the same PC can't receive the same data. BTW, it looks like O crashes if another app is already bound to the multicast address & port.

On output you can't select which interface it uses. For example, take the scenario of users with a radar connected to an Ethernet cable and a WiFi connection to their Actisense or Yacht Devices gateway. IIRC the Ethernet will have a lower route metric therefore by default, multicast traffic will be sent on the Ethernet network, which is probably not their intent.

There's probably heaps more and I haven't even thought about TCP, but my brain is fogged after a long flight so these few comments will suffice. I trust that Dave will bring some sanity to the discussion.

@rgleason
Copy link
Collaborator

rgleason commented Apr 6, 2024

With all this brain power (when not fogged) I could really use some help getting specific areas of the manual sorted: I got brain fog going in circles there, need fresh eyes:

Quick Start GNSS Setup and OS pages
Connections
Connections > Advanced
Connections > Data Sharing

I know you all have contributed to these pages, but we really need to get this as a simple step by step. I've taken annotated screenshots that will probably have to be redone eventually, but perhaps getting the manual whipped into shape will help with the thought process here?
Thanks so much.

@wholybee
Copy link
Contributor

wholybee commented Apr 6, 2024

I have been working on this the past few days, as an exercise to familiarize myself with the OCPN code, not expecting any changes to be accepted. But, I will share what I have done. Maybe some of these changes should be considered.

  1. Adjusted Alignment of columns for more consistent display between Serial/Network. It was distracting when column widths changed and shifted.
  2. The default network ports did not correctly change when changing protocol. The code would only change if the port field was empty, but the port was never empty because the dialog fills in port 10110 when it opens. Now, instead of checking if empty, it checks if the port is a default (10110, 2947, or 3000). If not a default, the user must have changed it, so leave it alone.
  3. The default port for SignalK was incorrect. It was 8375 but should be 3000. This was an issue with a user on Facebook.
  4. Changed the positioning of the "List Position" field so that it doesn't move around when changing between Serial and Network.
  5. Moved the Control Checksum and Garmin Mode checkbox below the Receive and Output Checkboxes.
  6. Added an "Advanced Options" checkbox. When unchecked, everything below the Receive Input and Output checkboxes are hidden.
  7. For UDP, only one of the Receive and Output checkboxes can be checked.
  8. Check the network to guess at better default IP addresses. The code looks for an adapter with a Gateway assigned. In most cases, probably only one adapter will have this.
    -For TCP/IP, the gateway address is the default. This is probably correct 50% of the time.
    -For UDP, if the Output checkbox is checked, the default IP address is the broadcast address, otherwise, the device's own IP address.
    -For GPSD and SignalK, the Gateway Address is used.
    -And I know everyone hated this, but a Multicast checkbox. When checked, the default address of 224.0.2.21 is used.
  9. In all cases, the IP Address field is left enabled, so anything can be entered.

You really have to play with it to see how well it provides reasonable defaults without getting in the way of entering whatever you want if needed. And the column realignment and advanced checkbox make it much more inviting and less overwhelming to a new user, while providing what most users will need in a simplified way.

Screenshot 2024-04-06 140232
Screenshot 2024-04-06 140302
Screenshot 2024-04-06 140241
Screenshot 2024-04-06 140251

@TwoCanPlugIn
Copy link
Contributor

Would someone care to answer the following:
What does "List position" mean or do ?
Are Talker Prefix and APB precision global or per connection ?

question

@bdbcat
Copy link
Member

bdbcat commented Apr 6, 2024

  1. List position determines the display order of this connection in the list. Counts like a human, 1 at the top, 9 at the bottom.
  2. I think tallker ID and APB precision are global, but I will verify.

@bdbcat
Copy link
Member

bdbcat commented Apr 7, 2024

Wjollybee..
Are your changes mockups, or actual wxWidgets code?

@wholybee
Copy link
Contributor

wholybee commented Apr 7, 2024 via email

@TwoCanPlugIn
Copy link
Contributor

List position determines the display order of this connection in the list. Counts like a human, 1 at the top, 9 at the bottom.

It doesn't seem to work as one would expect. I can set all of the connections to the same list position number (including zero).

Why not just have up/down arrows to change the order, similar to the attached dialog. In any case, other than the display order, does it do anything, such as affect the priority of sentences ?

ordered-list

@bdbcat
Copy link
Member

bdbcat commented Apr 7, 2024

" I can set all of the connections to the same list position number (including zero).'
I guess you can, and then you get undefined order.
I agree an up/down control would be nicer. An opportunity for a new OCPN code contribution.

Other than the "list position", this field has no other effect on this, or any other connection.

@bdbcat
Copy link
Member

bdbcat commented Apr 7, 2024

Wholybee...
Wow, nice work. The only think I don't love is the serial baudrate field hanging out to the side. This will be annoying on Android phone (portrait mode).
Other than that, looks good for testing and feedback. if you recast this as a Pull Request, that will let me and others take it for a spin.

@wholybee
Copy link
Contributor

wholybee commented Apr 7, 2024

I agree on the baud rate. I am on my boat now for a couple days. When I get back home I'll do a pull request.

@bdbcat
Copy link
Member

bdbcat commented Apr 11, 2024

Wholybee
I have built your PR (with a few source code fixes) for linux, and done some smoke-testing. Comments:
0. Overall, a clear improvement in the GUI. Less dislocating layout changes. Good.

  1. The "Advanced Options button seems to be a good idea. But, in other places in OCPN, we prefer to use a syntax like "More..." to show the options, toggling to "Less..." to hide the options.
  2. The adapterInfo notion adds more platform/system level dependency than I would like to see here. I think it might be sufficient to simply use the standard "0.0.0.0", etc. without probing the hardware. 99% of the time, that will be correct for users that will appreciate the auto-filled hint values. And as you say, the field is always editable for experts.

Thanks, looking forward to your comments.

@wholybee
Copy link
Contributor

Thank you. I can work on that next week. I'll remove the adapterInfo parts, change the IP addresses to 0.0.0.0, and change "advanced" to more/less.

Can you show me an example of the more/less for me to follow? I assume instead of a checkbox, it would be a button whose label changes?

@bdbcat
Copy link
Member

bdbcat commented Apr 11, 2024

That will be fine.
More/Less is used in the plugin manager. See the picture.
Could use a button, but I prefer the bare text, looking like an HTML tag.
Screenshot_20240411_125118

@wholybee
Copy link
Contributor

Ok, I have removed the adapterInfo networking stuff, changed the advanced checkbox to more/less, and a couple other minor fixes. For example, the control checksum and garmin options were displayed on some protocols where they shouldn't have been. It should have some more testing, particularly on canbus, bluetooth, and android devices, since I can't test those.

Also, this is my first pull request on github. Let me know if I do something wrong. I am learning that maybe I should have branched before committing changes and making the request.

@bdbcat
Copy link
Member

bdbcat commented Apr 19, 2024

OK, I'll review tomorrow, and probably merge.
Thanks

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

No branches or pull requests

9 participants