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

expereimental changes to connection edit dialog #3798

Merged
merged 4 commits into from
Apr 19, 2024
Merged

Conversation

wholybee
Copy link
Contributor

@wholybee wholybee commented Apr 9, 2024

Some ideas related to issue #3700
#3700 (comment)

Everything I have done should be considered experimental just to see some ideas. The networking code in AdapterInfo.cpp is pretty rough and targets windows, so that should be completely rewritten.

@bdbcat
Copy link
Member

bdbcat commented Apr 19, 2024

Wholybee...
As you see, there are some build problems on linux and Mac.
I have looked at some of those. Mainly concerning "call by reference" problems, easily fixed.
I'll merge and do that tonight, unless you have a chance to correct the code beforehand.

@bdbcat bdbcat merged commit ef71218 into OpenCPN:master Apr 19, 2024
1 of 6 checks passed
@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

gui/include/gui/connection_edit.h:154: trailing whitespace.
+
gui/src/AdapterInfo.cpp:71: trailing whitespace.

  • if (count != 4) {
    

gui/src/AdapterInfo.cpp:73: trailing whitespace.

  •     return 0;
    

gui/src/connection_edit.cpp:407: trailing whitespace.
+
gui/src/connection_edit.cpp:417: trailing whitespace.
+
gui/src/connection_edit.cpp:444: trailing whitespace.
+
gui/src/connection_edit.cpp:508: trailing whitespace.
+
gui/src/connection_edit.cpp:552: trailing whitespace.
+
gui/src/connection_edit.cpp:567: trailing whitespace.
+
gui/src/connection_edit.cpp:609: trailing whitespace.
+
gui/src/connection_edit.cpp:666: trailing whitespace.
+
gui/src/connection_edit.cpp:726: trailing whitespace.
+
gui/src/connection_edit.cpp:896: trailing whitespace.
+
gui/src/connection_edit.cpp:1172: trailing whitespace.
+
gui/src/connection_edit.cpp:1178: trailing whitespace.
+
gui/src/connection_edit.cpp:1362: trailing whitespace.
+
gui/src/connection_edit.cpp:1470: trailing whitespace.
+
gui/src/connection_edit.cpp:1914: trailing whitespace.
+
gui/src/connection_edit.cpp:1916: trailing whitespace.
+

@bdbcat
Copy link
Member

bdbcat commented Apr 20, 2024

Whollybee...
Merged and corrected small build problems.
Overall, looks great. A real improvement in UX. Thanks.
I see two small issues:

  1. Did we not agree that network UDP must support TX or RX, but not both? This needs fixing.
  2. On Network, "Use GRMN mode for input" is not valid. This mode can only apply to serial N0183 connections.

Thanks again.

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

I have a small question. This q instead of deeper own investigation:
This:

 //  For small displays, skip the "More" text.
 if (g_Platform->getDisplaySize().x < 80 * GetCharWidth()) MORE = "";

Does it mean small screen can't use filters and others normally hidden behind "More"?

@bdbcat
Copy link
Member

bdbcat commented Apr 20, 2024

Hmmm...
Good question. OP answer?

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

Another Q
Check UDP Multicast.
Address suggested then become 224.0.2.21.
I don't understand. What's that?

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@bdbcat
Copy link
Member

bdbcat commented Apr 20, 2024

Lets remove the small screen limitation, and we will see how it lays out on Android phones.
I do not yet have Android ready for O59, but coming soon.

I think the multicast flag is a good idea, in the "more" section. Let's keep it.

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

Sorry I didn't followed that discussion in all details but have tried to use it here.

My experiences:
Using UDP and 224.0.2.21 are not seen by others unless also that IP address is used on the receiver.
Using an address as "usual", 192.168.1.255 or 192.168.x.255 or even 224.x.x.255, are seen by all others. That is of course if others use input UDP 0.0.0.0 or localhost and the same port. This works for OCPN and other receivers like SignalK.

So the address 224.0.2.21 used on both tx and rx is when one for any (strange?) reason want to send only to designated receivers using the same address. And for performance use UDP instead of TCP.
To help first time users to set up a UDP transmit it may have been an alternative to use 224.x.x.255 instead of 224.0.2.21 and let all receivers use localhost or our default 0.0.0.0?
But there could be things I don't understand?

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

One comment if more input to this.
For a Signal K connection we may hide the "more" - "less" buttons. It's nothing there, see pict.

btw: The SK detection is not functional what I see. Neither with Auto detection enabled or not. I'm on Windows and checked for mdns in Wireshark while using the "Discover now". No Q Signlak activity.

bild

@nohal
Copy link
Collaborator

nohal commented Apr 20, 2024

Automatic server discovery has to be removed, does nothing.

@bdbcat
Copy link
Member

bdbcat commented Apr 20, 2024

Wholybee...
Are you planning to work these issues, or shall I? I worry about code collisions...

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

re:

With low bandwidth data and a small network on a boat, probably there is no reason to have multicast support at all.

Something like that was about to be my comment on this. Thanks.

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@Hakansv
Copy link
Contributor

Hakansv commented Apr 20, 2024

I've no clear view. That Auth token is seldom used so may confuse if not inside "more".
@nohal may have a clearer view?

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@wholybee
Copy link
Contributor Author

wholybee commented Apr 20, 2024 via email

@rgleason
Copy link
Collaborator

@bdbcat
From wholybee

@BDCat If network is selected, does the “Use GRMN mode for input” need to be cleared as well as hidden? That is, if the button is checked on a serial connection, and then the user changes the connection to Network, will the hidden but still checked button cause an issue?

@bdbcat
Copy link
Member

bdbcat commented Apr 21, 2024

While “Use GRMN mode for input” is a per-connection parameter, it is completely ignored for network connections.
So, no problem.

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

Successfully merging this pull request may close these issues.

None yet

5 participants