Re: [lwip-devel] core/timers.c with NO_SYS?

[Leon Woestenberg]

Hello Simon, Kieran,

On Sun, Jun 12, 2011 at 2:13 PM, address@hidden <address@hidden> wrote:
> Hi Leon,
>
> Leon Woestenberg wrote:
>>
>> I still don't understand; how can it be we *must* implement a sys_arch
>> layer function when NO_SYS is set?
>
> As I said, please don't let the name of that function confuse you. It's
> actually not a sys_arch layer function but a port function. In an embedded
> target, it's enough to set up a hardware counter to tick every millisecond
> and let sys_now() return that counter value. That has absolutely *nothing*
> to do with an OS layer. As such, the actual "bug" here, to me, is only a
> question of which name we give that function. I only chose sys_now() because
> it was already there and I didn't want to break old ports unnecessarily
> (which already might have implemented sys_now()).
>
> Aside from that, how do you propose to implement a generic timer framework
> without knowing the time? You do need a port-dependent function to get the
> time for that.
>
Depends on whether you want to support absolute time or not. In
previous versions, only relative time was needed (for timeouts).

>>
>> This makes a lower layer (the core API) depend on an optional higher
>> API abstraction layer (sys_arch). This breaks the architecture.
>>
>> This probably explains why NO_SYS_NO_TIMERS is there.... core/timers.c
>> doesn't seem to be a core architecture module this way.
>
> If you see it that way, the architecture is already broken as
> SYS_ARCH_PROTECT() is used in mem.c, memp.c and pbuf.c (and is included in
> many more, where it does not seem to be needed). Mem.c even uses semaphores,
> so I guess it's not a core architecture module, as well.
>
First of all "if you see it that way" doesn't sound very convincing.
The architecture has been like this from the first version of lwIP,
the sys_arch layer is fully optional, so core must not depend on it.

Agreed. We have sys_arch code in core. However, these functions and
macro's may all be no-ops on a NO_SYS implementation, thus there is no
hard dependency.

I am all in favor of removing it, I was strongly against this when it
was introduced.

Also, it is not needed to have sys_arch code in core, as I have shown
with various RTOS ports. The protection can be performed at a slightly
higher level, wrapping the core functions instead of mixing code.

The timers.c is the first module that requires the sys_now() to be
implemented. Sorry, I was indeed confused by the "sys_" prefix being a
sys_arch function.

> As such, it seems to me that we should sort out our sys.h/sys_arch.h
> handling instead of trying to argument away the need for a function
> returning the time or where it is defined.
>
Agreed.

> BTW: Bug #33544 is also a result of a suboptimal sys.h layout which defines
> some things to u8_t or void for NO_SYS==1.
>
I'll have a look into that.

Regards,
-- 
Leon