Re: [PATCHv5 11/13] net/lwip: connection between cmd and lwip apps

Ilias Apalodimas
2023-08-08T18:13:06Z

Hi Maxim

[...]

> > > +static int lwip_empty_tmo(void) { return 0; };
> > > +int (*ulwip_tmo)(void) = lwip_empty_tmo;
> > > +void ulwip_set_tmo(int (*tmo)(void))
> > > +{
> > > + ulwip_tmo = tmo;
> > > +}
> > > +
> > > +static void ulwip_clear_tmo(void)
> > > +{
> > > + ulwip_tmo = lwip_empty_tmo;
> > > +}
> > > +
> > > +static void ulwip_timeout_handler(void)
> > > +{
> > > + eth_halt();
> > > + ulwip_tmo();
> > > + net_set_state(NETLOOP_FAIL); /* we did not get the reply */
> >
> > I am not sure what I am reading here. You use callbacks a few lines above
> > to set a timeout function. But only set it for dhcp. On top of that the
> > function for DHCP has a case for a *successful* asignment of ip addresses.
> > Why are we setting the state to fail? And why are we complicating this by
> > assigning and removing callbacks if it's only used for dhcp?
> >
> >
> I need two time out callbacks here:
> 1. Trap rx polling loop if lwip application works too long. It is used when
> code goes to net_loop() code to poll rx packets
> and nobody interrupts this loop. This timeout is used for all cmds (lwip
> apps).
>
> 2. Trap lwip application after specific timeout and then check some state.
> That is case for DHCP, where LWIP DHCP does not have
> callback for changing state. And I need to know when to stop polling loop.

Yes but is there a reason to reassing those callback to a function ptr?
Just define them and use them

[...]

> > > + ret = ulwip_dhcp();
> > > +
> > > + net_set_timeout_handler(2000UL, ulwip_timeout_handler);
> > > +
> > > + ulwip_loop();
> > > + if (IS_ENABLED(CONFIG_CMD_TFTPBOOT)) {
> > > + ulwip_clear_tmo();
> > > +
> > > + filename = env_get("bootfile");
> > > + if (!filename) {
> > > + printf("no bootfile\n");
> > > + return CMD_RET_FAILURE;
> >
> > Why is this a failure? You just have the tftp command enabled but dont
> > want to download anything
> >
> > thanks, if dhcp did not return filename, but only IP, then nothing to
> download. It's not an error.

Yes but downloading a file is not mandatory, it depends on a DHCP option. If you
want to emulate this behaviour, you need to fail only if 'bootfile' is set but
cant be downloaded.

>
>
> > > + }
> > > +

Regards
/Ilias