Skip to content

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Nov 5, 2013

lowpan.c uses a custom list implementation and intermingles list handling with application logic.
This leads to errors like #299.

I had already implemented a very simple and tested generic list for olsr2, so add that to RIOT and make lowpan.c use it.

It's still missing documentation and RIOT coding style, I'll add that later if you agree with the general approach.

My application runs a lot more stable with it, so I guess this fixes #299

@OlegHahm
Copy link
Member

OlegHahm commented Nov 5, 2013

Can we not just use core/queue.c or extend it if necessary?

@miri64
Copy link
Member

miri64 commented Nov 5, 2013

Queue has to much extra data in it (namely data and priority). But using list as a superclass of queue (and moving it to core) could be something considered

@miri64
Copy link
Member

miri64 commented Nov 5, 2013

But doxygen documentation is missing and some coding conventions are not uphold.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't want multi line defines, so all of these functions should go as normal c functions.

The rule of thumb:use define for function aliases, but if you have more than one command use a normal function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but they operate directly on the variables inside the loop, the defines are there to make the functions easier to use and write.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, complex Macros make code really hard to debug. That was the intention behind the last point in:
https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions#wiki-functions

So, I'm with @mehlis here.

Use a generic list instead of the buggy custom one
@ghost ghost assigned miri64 Jan 7, 2014
@OlegHahm
Copy link
Member

OlegHahm commented Jan 7, 2014

Needs rebase and review.

@OlegHahm
Copy link
Member

I think it would be great if someone could maintain this PR.

@benpicco
Copy link
Contributor Author

benpicco commented Feb 6, 2014

Well if there is still interest in this list, what should be done to integrate it better with riot?
When using only simple_list_add_before it's also possible to keep the list ordered, but it's a bit hacky and I'm not sure if such deep changes to RIOT are desirable.

@Kijewski
Copy link
Contributor

Kijewski commented Feb 6, 2014

  • A common convention in multiline defines is to wrap everything in do { ... } while (0), so it appears to be a function that has to be terminated with a semicolon. All arguments should be complete upper case.
  • It is essential to put parentheses around every argument.
  • Arguments should not be evaluated multiple times if it can be helped. __extension__ __typeof__(HEAD) ____HEAD = (HEAD); etc would be useful. Otherwise you should use all caps for the macro name.
  • Underscore+lower case letter is a perfectly fine identifier, and could clash with applications (if list.h may be used by a user, too). The use of >=2 underscores and underscore+upper case letter is reserved for library use.
  • The complete absence of parens like in simple_list_find_cmp(...) is bound to fail.
  • I fail to see any advantage in defines like simple_list_clear(...).
  • There exists a multitude of list implementation in any thinkable license. Do we really need our own implementation?

@miri64
Copy link
Member

miri64 commented Feb 6, 2014

btw for sake of discussion: what was again wrong with clist in core?

@benpicco
Copy link
Contributor Author

benpicco commented Feb 6, 2014

what was again wrong with clist in core?

It only has add and remove, no search or for each functions, and it's double linked which wastes the a pointer for each element if not needed ;)

But really, I just wrote that list to match the requirements of my application, when I discovered an issue in RIOT allegedly originating in lowpan.c, I wanted to see what happened when I replaced the strange intermingled 'list' there with the one I already had, but not much changed except for that the code got more readable imho.

@mehlis
Copy link
Contributor

mehlis commented Feb 6, 2014

I'm with @Kijewski , so no need in this code. And I'm with @authmillenon : document all problems in clist, try to create test cases. Fix the problem.

As far as I know, no problems are known with clist. The problem is probably in the sixlowpan stack using it. (or there is no problem at all...)

@OlegHahm OlegHahm added this to the Release NEXT MAJOR milestone Feb 6, 2014
@miri64
Copy link
Member

miri64 commented Feb 6, 2014

@mehlis As far as I know sixlowpan is not using clist.

@mehlis
Copy link
Contributor

mehlis commented Feb 6, 2014

@authmillenon ah I see.
so we assume that there is a problem with sixlowpans own list implementation (whatever it is is): proposed solution switch to clist.

@mehlis
Copy link
Contributor

mehlis commented Feb 6, 2014

just to have it here: another possible candidate for a list implementation: https://github.com/jpfender/riot-gossip/blob/master/src/list.c (has other problems regarding malloc/free....)

@miri64
Copy link
Member

miri64 commented Feb 6, 2014

sixlowpan uses static arrays currently, but will change for the most things (addresses and prefixes mainly) next week.

@mehlis
Copy link
Contributor

mehlis commented Feb 24, 2014

partially implemented in #644, code not ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no memory left in handle_packet_fragment - when there still is plenty of memory left

5 participants