You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/09/23 19:25:12 UTC
[GitHub] [incubator-nuttx-apps] davids5 opened a new pull request #402: Master pr netconf
davids5 opened a new pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402
## Summary
Sister to https://github.com/apache/incubator-nuttx/pull/1886 - merge this APP pr second.
# netinit:Network Monitor add a polled option
Not all boards have in interrupt line from the phy to the Soc. This allows the phy to be polled for link status.
This may not work on all MAC/PHY combination that have mutually exclusive link management and operating
modes. The STM32F7 and LAN8742AI do not have such a limitation.
To use this option the arch must supply CONFIG_ARCH_PHY_POLLED and not CONFIG_ARCH_PHY_INTERRUPT.
If the network is not connected the phy init may time out and netinit_net_bringup will return with out completing DHCP or initializing the ntp client.
This change uses the lack of the DHCP lease, to contiune the netinit_net_bringup, once the interface does come up. It also will renew the lease over time, and will re run the dhcp request after a network down/up transition.
# netinit Net monitior support getting address from board
Add the use of CONFIG_BOARDCTL_NETCONF to retrieve the address configurations from the board. Allowing setting
static or dhcp addressing at run time.
# netinit:Net monitior support fallback to static IP
After CONFIG_NETINIT_FALLBACK dhcp attempts, fall back to the static IP provided by the board via CONFIG_BOARDCTL_NETCONF.
## Impact
None All options have Kconfig knobs to turn them off by default
## Testing
FMUV5X with a test vector of all the new Kconfig options.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495047572
##########
File path: netutils/netinit/netinit.c
##########
@@ -411,45 +519,26 @@ static void netinit_set_ipaddrs(void)
#endif
/****************************************************************************
- * Name: netinit_net_bringup()
+ * Name: netinit_dhcp()
*
* Description:
- * Bring up the configured network
+ * Renew or obtain a IP Address over DHCP
*
****************************************************************************/
-#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NETINIT_NETLOCAL)
-static void netinit_net_bringup(void)
-{
#ifdef CONFIG_NETINIT_DHCPC
+static int netinit_dhcp(struct dhcpc_state *ds)
+{
uint8_t mac[IFHWADDRLEN];
- struct dhcpc_state ds;
FAR void *handle;
-#endif
-
- /* Bring the network up. */
-
- if (netlib_ifup(NET_DEVNAME) < 0)
- {
- return;
- }
-
-#ifdef CONFIG_WIRELESS_WAPI
- /* Associate the wlan with an access point. */
-
- if (netinit_associate(NET_DEVNAME) < 0)
+ int ret = -1;
+#ifdef CONFIG_BOARDCTL_NETCONF
+ if ((g_netconf.flags & BOARDIOC_NETCONF_DHCP) == 0)
{
- return;
+ return 1;
Review comment:
What does the return value of 1 mean?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495075044
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
There are three nsh_consolemain() functions, each of them calls netinit_bringup() unconditionally. There is only one system console, but potentially many telnet NSH sessions. There are at least two ways to get multiple session:
- Telnet. But in this case, nsh_consolemain() will not be called. A special nsh_telnetmain() is called instead and it does not re-initialize the network.
- You can also create a new NSH session from the `nsh>` command line. This, I suspect will be a problem because I think that nsh_consolemain() (and, hence, netinit_bringup()) WILL be called again.
I don't see any protections in netinit_bringup() or netinit_configure() to make them idempotent. What does happen in this case? My guess is that you are correct to be concerned.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699042290
@davids5 I think the first two patches are reasonable, could you remove the last one?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino edited a comment on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino edited a comment on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699008903
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495045596
##########
File path: netutils/netinit/netinit.c
##########
@@ -357,6 +365,85 @@ static void netinit_set_macaddr(void)
# define netinit_set_macaddr()
#endif
+#ifdef CONFIG_BOARDCTL_NETCONF
+/****************************************************************************
+ * Name: netinit_get_ipaddrs
+ *
+ * Description:
+ * Setup IP addresses.
+ *
+ * For 6LoWPAN, the IP address derives from the MAC address. Setting it
+ * to any user provided value is asking for trouble.
+ *
+ ****************************************************************************/
+
+static int netinit_get_ipaddrs(void)
+{
+#ifdef CONFIG_NETINIT_DHCPC
+ bool use_dhcp = false;
+#endif
+ bool use_static = false;
+#ifdef CONFIG_NET_IPv4
+ struct in_addr addr;
+
+ int ret = boardctl(BOARDIOC_NETCONF, (uintptr_t) &g_netconf);
Review comment:
As I disucss in apache/incubator-nuttx#1886, I do not think it is appropriate to use boardctl() for this purpose when the established, mostly standard ways of getting this information. NuttX is ALL about conformance to standards and we must be faithful to that.
boardctl() is unfortunately necessary, but we cannot let it be an expedient, low-effort solution for system interface issues that already have standard solutions. We must to the work as necessary and not cut corners.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] xiaoxiang781216 commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495019024
##########
File path: netutils/netinit/netinit.c
##########
@@ -627,6 +655,18 @@ static int netinit_monitor(void)
}
#endif
+
+#ifdef CONFIG_NETINIT_DHCPC
+ /* We assume that the netinit_thread was able to get a lease
+ * If it did we set up the renew point at lease_time / 2
+ * if it did not the renew point is now.
+ */
+
+ DEBUGVERIFY(clock_gettime(CLOCK_REALTIME, &dhcprenew));
Review comment:
should we use time API to simplify the timing calculator?
##########
File path: netutils/netinit/netinit.c
##########
@@ -357,6 +365,85 @@ static void netinit_set_macaddr(void)
# define netinit_set_macaddr()
#endif
+#ifdef CONFIG_BOARDCTL_NETCONF
+/****************************************************************************
+ * Name: netinit_get_ipaddrs
+ *
+ * Description:
+ * Setup IP addresses.
+ *
+ * For 6LoWPAN, the IP address derives from the MAC address. Setting it
+ * to any user provided value is asking for trouble.
+ *
+ ****************************************************************************/
+
+static int netinit_get_ipaddrs(void)
+{
+#ifdef CONFIG_NETINIT_DHCPC
+ bool use_dhcp = false;
+#endif
+ bool use_static = false;
+#ifdef CONFIG_NET_IPv4
+ struct in_addr addr;
+
+ int ret = boardctl(BOARDIOC_NETCONF, (uintptr_t) &g_netconf);
Review comment:
why not support ipv6
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
don't need, global variable is already zerod
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699014953
> Not sure how I didn't see the nuttx changes PR or realize there would have to be one. That one's a little more active, I'll spend some time reviewing it since this change is meaningless until the other one is resolved.
I want that change to be made in a more standard and compatible way, using existing mechanisms, proper separation of application and OS functionality (DHCP is application functionality) and without using the KLUDGE boardctl() interface.
My concern is with the integrity and roadmap of the OS. I don't want quick-n-dirty changes when a proper system solution is required. That degrades the OS.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495066995
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
Sorry for the long, somewhat tangent reply.
I was curious about this comment. While I agree it is not necessary for the normal call vector, who says that `netinit_configure()` cannot be called a second time. In this case, I'd think ensuring a known state is a good idea.
I was curious if it was called anywhere twice, so I searched where `netinit_configure()` is called from. The only place it is _currently_ called is `netinit_bringup()` or `netinit_thread()` (which `netinit_bringup()` launches)
But where is `netinit_bringup()` called from?
`nsh_consolemain()`
Looking at the documentation I see:
```
* Name: nsh_consolemain (Normal character device version)
*
* Description:
* This interfaces may be to called or started with task_start to start a
* single an NSH instance that operates on stdin and stdout. This
* function does not normally return (see below).```
I think this raises a concern. If I wanted multiple NSH consoles, does this mean that my network gets re-configured each time any of them come up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] davids5 closed pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
davids5 closed pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495066995
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
I was curious about this comment. While I agree it is not necessary for the normal call vector, who says that `netinit_configure()` cannot be called a second time? In this case, I'd think ensuring a known state is a good idea.
I was curious if it was called anywhere twice, so I searched where `netinit_configure()` is called from. The only place it is _currently_ called is `netinit_bringup()` or `netinit_thread()` (which `netinit_bringup()` launches)
But where is `netinit_bringup()` called from?
`nsh_consolemain()`
Looking at the documentation I see:
```
* Name: nsh_consolemain (Normal character device version)
*
* Description:
* This interfaces may be to called or started with task_start to start a
* single an NSH instance that operates on stdin and stdout. This
* function does not normally return (see below).```
I think this raises a concern. If I wanted multiple NSH consoles, does this mean that my network gets re-configured each time any of them come up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495066995
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
I was curious about this comment. While I agree it is not necessary for the normal call vector, who says that `netinit_configure()` cannot be called a second time. In this case, I'd think ensuring a known state is a good idea.
I was curious if it was called anywhere twice, so I searched where `netinit_configure()` is called from. The only place it is _currently_ called is `netinit_bringup()` or `netinit_thread()` (which `netinit_bringup()` launches)
But where is `netinit_bringup()` called from?
`nsh_consolemain()`
Looking at the documentation I see:
```
* Name: nsh_consolemain (Normal character device version)
*
* Description:
* This interfaces may be to called or started with task_start to start a
* single an NSH instance that operates on stdin and stdout. This
* function does not normally return (see below).```
I think this raises a concern. If I wanted multiple NSH consoles, does this mean that my network gets re-configured each time any of them come up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495046881
##########
File path: netutils/netinit/netinit.c
##########
@@ -343,6 +365,85 @@ static void netinit_set_macaddr(void)
# define netinit_set_macaddr()
#endif
+#ifdef CONFIG_BOARDCTL_NETCONF
+/****************************************************************************
Review comment:
This should depend on the existence of etc/sysconfig/network-scripts/ifcfg. boardctl() should not be used as discussed in apache/incubator-nuttx#1886
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino commented on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699008903
>
>
> > I've looked at this twice now and nothing stands out. I'll let the others comments get responded to before approving.
>
> The use of boardctl() in this way is unacceptable. I discuss in the matching PR [apache/incubator-nuttx#1886](https://github.com/apache/incubator-nuttx/pull/1886)
Not sure how I didn't see the nuttx changes PR or realize there would have needed to be one. That ones a little more active, I'll spend some time reviewing it since this change is meaningless until the other one is resolved.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495047295
##########
File path: netutils/netinit/netinit.c
##########
@@ -411,45 +519,26 @@ static void netinit_set_ipaddrs(void)
#endif
/****************************************************************************
- * Name: netinit_net_bringup()
+ * Name: netinit_dhcp()
*
* Description:
- * Bring up the configured network
+ * Renew or obtain a IP Address over DHCP
*
****************************************************************************/
-#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NETINIT_NETLOCAL)
-static void netinit_net_bringup(void)
-{
#ifdef CONFIG_NETINIT_DHCPC
+static int netinit_dhcp(struct dhcpc_state *ds)
+{
uint8_t mac[IFHWADDRLEN];
- struct dhcpc_state ds;
FAR void *handle;
-#endif
-
- /* Bring the network up. */
-
- if (netlib_ifup(NET_DEVNAME) < 0)
- {
- return;
- }
-
-#ifdef CONFIG_WIRELESS_WAPI
- /* Associate the wlan with an access point. */
-
- if (netinit_associate(NET_DEVNAME) < 0)
+ int ret = -1;
Review comment:
What does the return value -1 mean? -EPERM?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495047572
##########
File path: netutils/netinit/netinit.c
##########
@@ -411,45 +519,26 @@ static void netinit_set_ipaddrs(void)
#endif
/****************************************************************************
- * Name: netinit_net_bringup()
+ * Name: netinit_dhcp()
*
* Description:
- * Bring up the configured network
+ * Renew or obtain a IP Address over DHCP
*
****************************************************************************/
-#if defined(NETINIT_HAVE_NETDEV) && !defined(CONFIG_NETINIT_NETLOCAL)
-static void netinit_net_bringup(void)
-{
#ifdef CONFIG_NETINIT_DHCPC
+static int netinit_dhcp(struct dhcpc_state *ds)
+{
uint8_t mac[IFHWADDRLEN];
- struct dhcpc_state ds;
FAR void *handle;
-#endif
-
- /* Bring the network up. */
-
- if (netlib_ifup(NET_DEVNAME) < 0)
- {
- return;
- }
-
-#ifdef CONFIG_WIRELESS_WAPI
- /* Associate the wlan with an access point. */
-
- if (netinit_associate(NET_DEVNAME) < 0)
+ int ret = -1;
+#ifdef CONFIG_BOARDCTL_NETCONF
+ if ((g_netconf.flags & BOARDIOC_NETCONF_DHCP) == 0)
{
- return;
+ return 1;
Review comment:
What does the return value of 1 mean? Any non-negative is the usual return value for success.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] davids5 commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495092902
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
@antmerlino Yes that make sense, I was only looking at the init thread case and overlooked it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] patacongo commented on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699005302
> I've looked at this twice now and nothing stands out. I'll let the others comments get responded to before approving.
The use of boardctl() in this way is unacceptable. I discuss in the matching PR apache/incubator-nuttx#1886
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] antmerlino commented on a change in pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#discussion_r495066995
##########
File path: netutils/netinit/netinit.c
##########
@@ -533,6 +554,10 @@ static void netinit_configure(void)
#ifndef CONFIG_NETINIT_NETLOCAL
/* Bring the network up. */
+#ifdef CONFIG_NETINIT_DHCPC
+ memset(&g_ds, 0, sizeof(g_ds));
Review comment:
I was curious about this comment. While I agree it is not necessary for the normal call vector, who says that `netinit_configure()` cannot be called a second time? In this case, I'd think ensuring a known state is a good idea.
I was curious if it was called anywhere twice, so I searched where `netinit_configure()` is called from. The only place it is _currently_ called is `netinit_bringup()` or `netinit_thread()` (which `netinit_bringup()` launches)
But where is `netinit_bringup()` called from?
`nsh_consolemain()`
Looking at the documentation I see:
```
* Name: nsh_consolemain (Normal character device version)
*
* Description:
* This interfaces may be to called or started with task_start to start a
* single an NSH instance that operates on stdin and stdout. This
* function does not normally return (see below).
```
I think this raises a concern. If I wanted multiple NSH consoles, does this mean that my network gets re-configured each time any of them come up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx-apps] davids5 commented on pull request #402: Network Monitor: Runtime selection of DHCP/Static IP with fall back set by the board, and a polled mode for HW lacking a PHY interrupt
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #402:
URL: https://github.com/apache/incubator-nuttx-apps/pull/402#issuecomment-699020665
> > Not sure how I didn't see the nuttx changes PR or realize there would have to be one. That one's a little more active, I'll spend some time reviewing it since this change is meaningless until the other one is resolved.
>
> I want that change to be made in a more standard and compatible way, using existing mechanisms, proper separation of application and OS functionality (DHCP is application functionality) and without using the KLUDGE boardctl() interface.
>
> My concern is with the integrity and roadmap of the OS. I don't want quick-n-dirty changes when a proper system solution is required. That degrades the OS.
Thank you all for all the feedback.
While this solve the problem on a small system using netinit thread, it is far from what is needed as a general solution So I am closing this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org