buglwIP - A Lightweight TCP/IP stack - Bugs: bug #25608, Unable to use NO_SYS=1 and...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #25608: Unable to use NO_SYS=1 and SYS_LIGHTWEIGHT_PROT=1

Submitted by:  Bill Auerbach <billauerbach>
Submitted on:  Tue 17 Feb 2009 02:55:15 PM UTC  
 
Category:  pbufs Severity:  3 - Normal
Item Group:  Change Request Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <goldsimon>
Open/Closed:  Closed Planned Release:  None
lwIP version:  CVS Head

( Jump to the original submission)

Mon 27 Apr 2009 08:15:40 PM UTC, comment #21: 

Done by moving sys_now out of the #if NO_SYS and removing the typedef for sys_prot_t from #if NO_SYS (can be defined in cc.h now). Please reopen if you disagree.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 27 Apr 2009 08:05:15 PM UTC, comment #20: 

I'll check it in as I think it really is minor: I thought about incompatibilities because sys_prot_t is not defined any more, but if SYS_LIGHTWEIGHT_PROT is not defined (as it might well be up to now for the ones using NO_SYS=1), it does not matter if sys_prot_t is defined to u8_t or not defined at all.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 27 Apr 2009 07:48:28 PM UTC, comment #19: 

Thanks. I for one am fine with that approach and can test a build with it implemented like that.  I know these kinds of things are truly minor.

Bill Auerbach <billauerbach>
Mon 27 Apr 2009 07:43:36 PM UTC, comment #18: 

I guess this is a more generic problem since it also affects the TCP timestamp option (not working with NO_SYS=1 right now) since it needs sys_now().

Although an extra header could be useful to cleanly separate between "with OS" and "without OS", I think for now it could be enough to move the inclusion of sys_arch.h out of the #if NO_SYS and to remove the dummy typedef for sys_prot_t.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 08 Apr 2009 09:24:03 PM UTC, comment #17: 

From Kieren's message above: "I can see what Bill is getting at. It looks to me as though the code in sys.h has been written to assume that if you're using NO_SYS=1, you are not using SYS_LIGHTWEIGHT_PROT=1: the SYS_LIGHTWEIGHT_PROT stuff requires sys_arch.c, but we only include sys_arch.h if NO_SYS=0"

I had a problem redefining them (or I wouldn't have needed to post here).  The problem is the typedef, and Jim knew of it and posted his workaround.  What I remember is that it doesn't work as-is, not that it's a huge problem.  We NO_SYS=1 people are in the minority.

Bill

Bill Auerbach <billauerbach>
Wed 08 Apr 2009 07:58:13 PM UTC, comment #16: 

Bill, last time I checked I could use the defines with NO_SYS=1, not by implementing the functions but by overriding the defines (sys.h checks if they are already defined and does not re-define them).

That leaves us with the question why the defines are in sys.h - maybe they should be moved elswhere.

Jim, I don't see how the stack would work with LIGHTWEIGHT_PROT=0 and NO_SYS=0: all the pools and some other things would be unprotected, since they intentionally don't use semaphores (too much overhead for a -- or ++). In my opinion, LIGHTWEIGHT_PROT is required when using a SYS.

You're right right on NO_SYS=1, however: you only need LIGHTWEIGHT_PROT when using an ISR-enabled driver, you don't need it in a pure polling environment.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 08 Apr 2009 07:04:04 PM UTC, comment #15: 

Maybe I meant to say that they belong in cc.h, not sys.h.  Since I don't use a "sys", I don't focus on either of these files. :-)

Bill

Bill Auerbach <billauerbach>
Wed 08 Apr 2009 06:59:24 PM UTC, comment #14: 

The point was, or is, in a nutshell, that one cannot use the protection macros as is with NO_SYS=1.  The use of these macros shouldn't be prevented by using NO_SYS=1, because they can still be used in ISRs as you noted.

My question was why not add #include sys_arch.h to sys.h is wrong.  The statement should have been that the protection macros are misplaced and they belong in sys.h.  As noted, they don't require the use of an OS so they don't belong in an OS related file.  IMO anyway.

Bill

Bill Auerbach <billauerbach>
Wed 08 Apr 2009 06:30:37 PM UTC, comment #13: 

>Kieran, if protection isn't NO_SYS dependent, doesn't it make sense to move the protection #defines outside of the #if NO_SYS #else #endif group entirely?


I believe they already are outside of this group.

>At the top of sys.h, I don't understand why not add #include "sys_arch.h" for both cases of NO_SYS. The system requirements in sys.h should bring in any architecture requirements or overrides for both cases. sys_arch.h should take care of its NO_SYS business.


I think the point of the lightweight protection scheme was to allow the stack to support ISR-based drivers without requiring the features of a full-blown OS (queues, mailboxes, semaphores). I am not sure it makes much sense to implement a scheme for SYS_LIGHTWEIGHT_PROT with NO_SYS=0, but it isn't prevented currently that I can see.

In theory anyway, the sys_arch.h header represents only the OS interface portion of the architecture (there is also cc.h and cpu.h for the other vagaries of your target). I can understand the reasoning behind not including sys_arch.h if you have no OS.

Bottom line - there is one issue that could easily be cleaned up...  the comment in the SYS_ARCH_DECL_PROTECT section of sys.h indicates that the type of the protection variable can be overridden in sys_arch.h - which is not true when lightweight protection is most likely to be used (when NO_SYS=1). Maybe the comment should be changed?

Jim Pettinato <jim_pettinato>
Project Member
Tue 03 Mar 2009 06:51:16 PM UTC, comment #12: 

What I meant was that in the NO_SYS=1 section in sys.h, there are some typedefs that would also have to be removed if including sys_arch.h. Of course we could have the ports use #if NO_SYS in sys_arch.h, but that would be a break to the existing ports (which we should avoid if it isn't necessary).

An alternative would be to always include sys_arch.h and rely on the typedefs there (even if not needed).

I think I remember having moved the code for that to cc.h, where it is always included. The baseline of the current implementation seems to be that sys_arch.h provides OS funcitonality, which is not used with NO_SYS=1. I don't know if we should change this but I also remember finding it quite funny when starting with lwIP...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Tue 03 Mar 2009 05:07:00 PM UTC, comment #11: 

Kieran, if protection isn't NO_SYS dependent, doesn't it make sense to move the protection #defines outside of the #if NO_SYS #else #endif group entirely?

At the top of sys.h, I don't understand why not add #include "sys_arch.h" for both cases of NO_SYS.  The system requirements in sys.h should bring in any architecture requirements or overrides for both cases.  sys_arch.h should take care of its NO_SYS business.

BTW, my sys_arch.h does have NO_SYS=1 and NO_SYS=0 sections for the time that I might have a NO_SYS=0 port for the same architecture.  To be complete, I think a port which defines an architecture in sys_arch should allow for both NO_SYS=0 and NO_SYS=1 cases.  In fact, I think the less the core files depend on NO_SYS the better.

Bill Auerbach <billauerbach>
Tue 03 Mar 2009 04:52:11 PM UTC, comment #10: 

Your "shortcut" and adding the #defines to lwipopts.h is the only way to implement the protection macros without modifying lwip base files.  Thanks for the tip - I didn't think about just not using the typedef at all. :-)

Bill Auerbach <billauerbach>
Tue 03 Mar 2009 04:51:04 PM UTC, comment #9: 

I can see what Bill is getting at.  It looks to me as though the code in sys.h has been written to assume that if you're using NO_SYS=1, you are not using SYS_LIGHTWEIGHT_PROT=1: the SYS_LIGHTWEIGHT_PROT stuff requires sys_arch.c, but we only include sys_arch.h if NO_SYS=0

There are two ways around this:
 - restructure the code in sys.h to better reflect what it seems to be currently assuming by moving the SYS_LIGHTWEIGHT_PROT stuff into the NO_SYS=0 section; or
 - restructure the code in sys.h to allow SYS_LIGHTWEIGHT_PROT to be used with NO_SYS=1, i.e. what Bill is requesting.

Let's assume we try and do what Bill needs: Simon can you give some more detail of what you think the problem would be with including sys_arch.h in the NO_SYS=1 section of sys.h?  One possibility might be to use "#if NO_SYS in sys_arch.h, which although it makes the port-defined files a little more messy, would allow the port more flexibility in what it choses to define, and what can be left to be dummy-defined by sys.h.

Kieran Mansley <kieranm>
Project Member
Tue 03 Mar 2009 04:02:59 PM UTC, comment #8: 

Okay, I see why the u32_t now... level and mask, etc.

I agree we shouldn't assume anything about architecture-specific implementation layers. And I didn't mean to suggest anyone should be forced into using types that are inefficient for their architecture. What I was trying to say was I think that you should be able to use the native data type together with my 'shortcut' to achieve your desired result.

Jim Pettinato <jim_pettinato>
Project Member
Tue 03 Mar 2009 03:53:57 PM UTC, comment #7: 

No, it needs 32 bits to store the current interrupt state which is then set to 0.  To restore interrupts, this 32-bit value is written back to the register to enable only those interrupts that were on to begin with.

We shouldn't assume anything about how an arch will implement something that is arch-specific, including data types.  I overrode err_t because a u32_t on my previous system was more efficient than the default s8_t.  Sure, it may be negligible, but should anyone be forced into using types or methods inefficient for their architecture?

Bill Auerbach <billauerbach>
Tue 03 Mar 2009 03:20:54 PM UTC, comment #6: 

Does your arch really have more than 64k interrupt levels?

The sys_prot_t token isn't even parsed if you define SYS_ARCH_PROTECT in lwipopts.h.

I would think you could do the same thing I did, changing the 'int lev;' to 'u32_t lev;' and be done.

Note that I have not yet migrated to 1.3.0, maybe something has changed since 1.2.x that would prevent this shortcut from working?

Jim Pettinato <jim_pettinato>
Project Member
Tue 03 Mar 2009 02:56:16 PM UTC, comment #5: 

Jim, you didn't need to change sys_prot_t?  I do since my arch needs a u32_t.

Bill Auerbach <billauerbach>
Tue 03 Mar 2009 02:51:03 PM UTC, comment #4: 

I have the SYS_ARCH_PROTECT macros defined in my lwipopts.h file. It works fine, though I agree that it would be 'cleaner' if they were in sys_arch.h.

-------------------------------------------------------------
#define NO_SYS 1

#include "ei_di_in.h"  // prototype for setintlvl() function

// using int masking (no mutexes, etc)
#define SYS_LIGHTWEIGHT_PROT 1  

#define SYS_ARCH_DECL_PROTECT(lev) int lev;
// Define SYS_ARCH_PROTECT to mask just ETH0 interrupts
#define SYS_ARCH_PROTECT(lev) lev = setintlvl(2);
#define SYS_ARCH_UNPROTECT(lev) setintlvl(lev);

-------------------------------------------------------------

Jim Pettinato <jim_pettinato>
Project Member
Tue 03 Mar 2009 02:11:09 PM UTC, comment #3: 

For NO_SYS=1:

1) sys.h defines sys_prot_t.  It shouldn't because the arch layer should be responsible for this in sys_arch.h.  Or it needs an #ifndef wrapper, but then where will sys_prot_t be defined?

2) Without including sys_arch.h in the NO_SYS=1 section, how/where do you expect sys_prot_t, SYS_ARCH_PROTECT and the other macros to be defined?

Bill Auerbach <billauerbach>
Sun 01 Mar 2009 05:32:55 PM UTC, comment #2: 

Having a look over sys.h, I just thought it would be nice to define SYS_PROT_xxx as macros, not as functions like it is now.

However, including sys_arch.h in the NO_SYS=1 section is not such a good idea, as it prevents us from dummy-defining some typedefs and functions...

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 18 Feb 2009 01:44:08 PM UTC, comment #1: 

This can be accomplished by 2 changes to sys.h:

1) Add #include "lwip/sys_arch.h" in the #if NO_SYS section

2) Remove typedef u8_t sys_prot_t; so that the sys_arch.h file can provide it.

And a note: SYS_LIGHTWEIGHT_PROT must be 0.

Bill Auerbach <billauerbach>
Tue 17 Feb 2009 02:55:15 PM UTC, original submission:  

In sys.h, the sys_arch.h #include is bypassed when NO_SYS=1 preventing one from using SYS_LIGHTWEIGHT_PROT and the PROTECT macros.  pbuf.c and memp.c use PROTECT macros which are needed when NO_SYS=1 as well to allow calling pbuf_free from an Interrupt Service Routine.

Bill Auerbach <billauerbach>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by kieranm (Posted a comment)
  • -email is unavailable- added by jim_pettinato (Posted a comment)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by billauerbach (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-04-27 goldsimon StatusIn Progress => Fixed
        Open/ClosedOpen => Closed
    2009-04-27 goldsimon StatusNone => In Progress
        Assigned toNone => goldsimon

    Back to the top


    Powered by Savane 3.5