Ethernet : Added support in ethernet #10124

garfieldG · 2018-08-02T13:50:34Z

-new network_manager module gives the ability to control Ethernet related (as ip,mac and etc...) through parameters

Additional context
-To use network_manager in rcS add the line "network_manager start" before mavlink

garfieldG · 2018-08-02T13:52:26Z

dagar · 2018-08-02T14:45:11Z

A couple initial thoughts.

Running network manager as a task is a huge waste of memory

I don't have a better solution in mind at the moment, but spreading network configuration across parameters feels pretty awkward. Do you actually need it to be runtime configurable like this, or is it just a convenient way to make it persistent?

garfieldG · 2018-08-02T15:05:12Z

@dagar network manager does run as a task, but after setting all the Ethernet related it exits immediately.
It is a convenient way, not a must, you can always set it in defconfig, but it does make it simpler when you have more than one drone in one subnet (no network manager - you'll need to change each drone it's mac and IP in the defconfig, compile it differently for each drone and use untagged version)

davids5

@garfieldG This is cool! Thank you.

davids5 · 2018-08-02T15:02:08Z

src/modules/network_manager/network_manager_main.cpp

@@ -0,0 +1,241 @@
+/****************************************************************************


Why is this structured this way?

This seems like it should a new file in the common stm32 board code and called from init function based on #define values for IP and MAC. A wrapper systemcmd could be added and a init script call could be done from rcS or read from SD card file to set mac, ip etc.

@davids5
Hi,

So some thoughts after reading your comment.

There is no need to add a file in order to use "#define" values , the define values are already used, the network manager only uses the parameters values, if someone wants to use the "#define" values then there's no need in network manager or similar.
The "#define" values are set in defconfig:
-CONFIG_NSH_IPADDR
-CONFIG_NSH_DRIPADDR
-CONFIG_NSH_NETMASK
-CONFIG_NSH_MACADDR

SD card can be changed or moved while parameters are per plane.
From my experience it is easy to use the Ethernet related values as parameters.
You can change it very easily, no need to take the SD off, change it and put it back.
If you're not sure what the IP is (or some other value) you can always look in a program like QGC, and again no need to read values from a file saved on the SD.
It's quick and easy to use (especially in case of more than one person using the same plane and more than one plane) and I do think that the values should be changed through parameters.

@garfieldG - Thank you for the perspective. I always am looking from a deeply embedded point of view at the cost of a feature set on resources. So it is good from me to have your perspective from a use-case perspective. Thank you for the changes for the defines.

davids5 · 2018-08-02T15:08:07Z

src/modules/mavlink/mavlink_receiver.cpp

@@ -2702,7 +2711,11 @@ MavlinkReceiver::receive_start(pthread_t *thread, Mavlink *parent)
 	param.sched_priority = SCHED_PRIORITY_MAX - 80;
 	(void)pthread_attr_setschedparam(&receiveloop_attr, &param);

+#ifdef CONFIG_NET


This would be better structured as

#if !defined(CONFIG_NET) # define NET_ADDED_STACK 0 # else # define NET_ADDED_STACK 1360 #endif

At the top of file or header
and use
pthread_attr_setstacksize(&receiveloop_attr, PX4_STACK_ADJUSTED(2840+NET_ADDED_STACK ));
here

davids5 · 2018-08-02T15:09:51Z

src/modules/mavlink/mavlink_main.cpp

@@ -2683,7 +2763,11 @@ Mavlink::start(int argc, char *argv[])
 	px4_task_spawn_cmd(buf,
 			   SCHED_DEFAULT,
 			   SCHED_PRIORITY_DEFAULT,
+#ifdef CONFIG_NET


Same as comment as below - in general where possible 1 ifdef to configure the deltas and then no ifdefs in the code is a cleaner approach.

garfieldG · 2018-08-03T17:08:42Z

@davids5
Thanks for the response!
I'll look into it on sunday

LorenzMeier · 2018-08-05T10:18:50Z

Parameters make total sense, yes.

LorenzMeier · 2018-08-05T10:25:03Z

src/modules/mavlink/mavlink_main.cpp

+			break;
+
+//multicast
+#if defined(CONFIG_NET_IGMP) && defined(CONFIG_NET_ROUTE)


This is NuttX specific but probably also works on Linux. So probably best to do under one define (as @davids5 suggested).

LorenzMeier · 2018-08-05T10:26:20Z

src/modules/mavlink/mavlink_main.cpp

@@ -243,7 +249,7 @@ Mavlink::Mavlink() :
 	_rate_tx(0.0f),
 	_rate_txerr(0.0f),
 	_rate_rx(0.0f),
-#ifdef __PX4_POSIX
+#if defined(CONFIG_NET) || defined(__PX4_POSIX)


Creating a new define __PX4_NETWORK and setting that above for when NuttX networking is enabled or POSIX is available is cleaner.

LorenzMeier · 2018-08-05T10:27:23Z

src/modules/mavlink/mavlink_main.cpp

-
-		ret = sendto(_socket_fd, _network_buf, _network_buf_len, 0,
-			     (struct sockaddr *)&_src_addr, sizeof(_src_addr));
+#ifdef CONFIG_NET


Isn't this also true for POSIX? The source address has to be initialized.

LorenzMeier · 2018-08-05T10:28:59Z

@dagar Would you want to help this to get to completion so you can leverage this as well on your F7 unit?

garfieldG · 2018-08-05T12:23:06Z

@davids5
Added defines as requested

garfieldG · 2018-08-05T13:39:58Z

@LorenzMeier Thanks! Actually not sure where should I create the new define. Maybe whenever __PX4_POSIX is defined then define CONFIG_NET?

bkueng

Nice work @garfieldG ! I'm interested to know the general resource usage of the NuttX networking stack. On what board are you testing this?
What is the CPU load?
By how much does RAM usage increase?

bkueng · 2018-08-06T09:13:30Z

src/modules/network_manager/network_manager_main.cpp

+
+void NetworkManager::task_main()
+{
+	_param_sub = orb_subscribe(ORB_ID(parameter_update));


You're right, the initial thought was to enable IP changes after startup, but it was canceled afterwards due to problems we encountered

bkueng · 2018-08-06T09:14:34Z

src/modules/network_manager/network_manager_main.cpp

+
+int NetworkManager::start(int argc, char *argv[])
+{
+	_taskmap_id = px4_task_spawn_cmd("network_manager", SCHED_DEFAULT, SCHED_PRIORITY_DEFAULT, 1200,


This is not needed, as the task exits right after configuration. Can you directly execute the config inside the main task? Or is it something that takes a long time?

I don't think it's needed.
It's here 'cause NetworkManager was supposed to be a running task that can handle changes with parameters, but it was changed later on.

bkueng · 2018-08-06T09:18:40Z

src/modules/network_manager/network_params.c

+ * @min 0
+ * @max 255
+ */
+PARAM_DEFINE_INT32(ETH_IP_ADDR_0, 192);


Can you please use a single parameter for the IP and the Gateway? Both are 4 bytes each and fit into a single parameter.
We should then make sure QGC learns how to display IP addresses (@DonLakeFlyer @dogmaphobic ) and add the metadata.

It was structured like this because of the GUI and the fact that one parameter can be a very long number and it seemed like less comfortable solution. Do you think it's better to use a single parameter? It was a real conflict.

the fact that one parameter can be a very long number and it seemed like less comfortable solution.

Yes exactly. That's why we should to extend QGC so that it shows the IP in the form 192.168.1.1.

Well, there's a problem doing so...
Let's say we wan't to change the IP parameter value to 192.168.0.1 with the param set command.
The actual value that will be stored is 127.255.255.255 (LONG_MAX), I suspect the problem is with the do_set function that uses strtol.

Oh! So we would need to fix the parameters. But that would cause more bloat to go to 64 bits (unless the union already is that big - IDNR)

I think this further reinforces @dagar
#10124 (comment) and also suggest the parameters need some design effort for isolation (things stores but not user facing) and scalability.

We should then make sure QGC learns how to display IP addresses (@DonLakeFlyer @dogmaphobic ) and add the metadata.

That would require a parameter type of IP address to be added to mavlink and then flow through to QGC changes.

why not mac using 2 parameters?

We can do that too but it's more than 4 bytes. A 64 bit param type would be helpful.

For the param set we can easily extend it to accept the form x.y.z.w.

@DonLakeFlyer

That would require a parameter type of IP address to be added to mavlink and then flow through to QGC changes.

Why mavlink? We can do it through the unit in the param xml metadata. It would still be an INT32 as underlying type.

Oh! So we would need to fix the parameters. But that would cause more bloat to go to 64 bits

Why not just using uint32? why 64?

For the param set we can easily extend it to accept the form x.y.z.w.

I think this is my preferred idea, much more user friendly

We can do that too but it's more than 4 bytes. A 64 bit param type would be helpful.

Yes, it would be. But if for some reason we don't want a 64 bit param we can use 2 32 bit param for mac, do you think it's a good idea? or is it just another thing that will complicate the user experience?

But if for some reason we don't want a 64 bit param we can use 2 32 bit param for mac, do you think it's a good idea? or is it just another thing that will complicate the user experience?

I think that would complicate the user experience too much.

dagar · 2018-08-06T19:23:50Z

I don't think parameters really make sense for this kind of configuration, but it's our only option at the moment. We need something like a mavlink message to configure the network interface atomically with a different storage backend internally. Something for another day...

I'll test this on the F7 board I have with ethernet.

garfieldG · 2018-08-07T12:30:39Z

@bkueng I tested it on a costume board.
I can't tell you how much the RAM will increase, but it will increase.
It could be an issue with f4 and I do recommend to eliminate unnecessary.
In f7 it should be OK.

davids5 · 2018-08-07T14:58:18Z

Seeing that the CONFIGxxx come from the defconfig and nuttx has to be built with the config, We have 2 options. 1) maintain and carry a separate board config target with _ehternet or 2) have a process to concatenates the net config with all that is needed by NuttX for a given release I think it is worth pointing out that the current nuttx release has had many Networking related changes since the fork point. The process to update to current nuttx has to be scheduled and supported. But first we need the ability to QA the upgrade on all platforms.

dagar · 2018-08-08T18:02:09Z

As discussed on the dev call I propose splitting off the mavlink changes into a separate PR we can test, review, merge ASAP and the network_manager discussion can continue here.

garfieldG · 2018-08-09T12:27:06Z

@dagar Added a new PR with Mavlink changes

garfieldG · 2018-08-15T07:28:22Z

@davids5 @dagar @bkueng
Did we get to any decision regarding the network manager?
Do you want me to change the IP and the gateway to two one parameter each? What are we doing regarding the parameters? Are we adding support in x.y.z.w format or will we use it as is and use the 2's complement calc?

-new network_manager module gives the ability to control ethernet related (as ip,mac and etc...) through parameters

julianoes · 2019-07-09T07:39:01Z

@davids5 what's the status here?

davids5 · 2019-07-09T12:25:48Z

@julianoes IIRC @dagar had an issue with the use of param for the IP address, but no other solutions was developed.

julianoes · 2019-07-09T12:46:11Z

When I read it I see that @garfieldG 's questions never got answered.

stale · 2019-10-09T06:49:14Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dagar · 2019-10-15T14:58:17Z

FYI @dinomani

stale · 2020-01-14T09:07:28Z

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

dagar self-requested a review August 2, 2018 14:37

dagar added the Admin: Enhancement (improvement) 💡 label Aug 2, 2018

davids5 requested changes Aug 2, 2018

View reviewed changes

LorenzMeier reviewed Aug 5, 2018

View reviewed changes

bkueng reviewed Aug 6, 2018

View reviewed changes

Ethernet : Added new module to support Ethernet parameters

67dee4d

-new network_manager module gives the ability to control ethernet related (as ip,mac and etc...) through parameters

garfieldG force-pushed the Ethernet branch from c6b3e88 to 67dee4d Compare August 16, 2018 09:32

dagar force-pushed the pr-av_x-ethernet branch from 52ab1b2 to a438f0e Compare August 18, 2018 14:03

dagar force-pushed the pr-av_x-ethernet branch 3 times, most recently from 8f9db90 to 03a56a2 Compare September 6, 2018 17:12

weekly-digest bot mentioned this pull request Jan 27, 2019

Weekly Digest (20 January, 2019 - 27 January, 2019) #11307

Closed

stale bot added the Admin: Wont fix label Jun 24, 2019

weekly-digest bot mentioned this pull request Jun 30, 2019

Weekly Digest (23 June, 2019 - 30 June, 2019) #12374

Closed

stale bot closed this Jul 8, 2019

julianoes reopened this Jul 9, 2019

stale bot removed the Admin: Wont fix label Jul 9, 2019

PX4 deleted a comment from stale bot Jul 11, 2019

weekly-digest bot mentioned this pull request Jul 14, 2019

Weekly Digest (7 July, 2019 - 14 July, 2019) #12474

Closed

stale bot added the stale label Oct 9, 2019

weekly-digest bot mentioned this pull request Oct 13, 2019

Weekly Digest (6 October, 2019 - 13 October, 2019) #13171

Closed

stale bot removed the stale label Oct 15, 2019

weekly-digest bot mentioned this pull request Oct 20, 2019

Weekly Digest (13 October, 2019 - 20 October, 2019) #13236

Closed

stale bot added the stale label Jan 14, 2020

garfieldG closed this Feb 23, 2020

garfieldG mentioned this pull request Mar 23, 2020

Added support in Mavlink Ethernet channel parameters #14460

Merged

		@@ -0,0 +1,241 @@
		/****************************************************************************

Ethernet : Added support in ethernet #10124

Ethernet : Added support in ethernet #10124

Conversation

garfieldG commented Aug 2, 2018 • edited by AuterionWrikeBot Loading

garfieldG commented Aug 2, 2018

dagar commented Aug 2, 2018

garfieldG commented Aug 2, 2018 • edited Loading

davids5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garfieldG commented Aug 3, 2018

LorenzMeier commented Aug 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LorenzMeier commented Aug 5, 2018

garfieldG commented Aug 5, 2018

garfieldG commented Aug 5, 2018

bkueng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Aug 6, 2018

garfieldG commented Aug 7, 2018

davids5 commented Aug 7, 2018

dagar commented Aug 8, 2018

garfieldG commented Aug 9, 2018

garfieldG commented Aug 15, 2018

julianoes commented Jul 9, 2019

davids5 commented Jul 9, 2019

julianoes commented Jul 9, 2019

stale bot commented Oct 9, 2019

dagar commented Oct 15, 2019

stale bot commented Jan 14, 2020

garfieldG commented Aug 2, 2018 •

edited by AuterionWrikeBot

Loading

garfieldG commented Aug 2, 2018 •

edited

Loading