-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
README.md: improve feature list #21059
base: master
Are you sure you want to change the base?
Conversation
9d62e4f
to
4e8a001
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, thanks for picking this up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the incompletness indicating ...
we do not claim completeness -> incompleteness is assumed (as ussual with such lists)
2bc5d86
to
b1a1a9f
Compare
b1a1a9f
to
570f97b
Compare
|
||
* Security Features | ||
- OTA updates via SUIT | ||
- PSA Cryptographic API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all in to show the PSA here, but the module still says "This implementation is not complete and not yet thoroughly tested. Please do not use this module in production, as it may introduce security issues."
Maybe it's time to drop that warning, if we feel comfortable to showcase it in the readme? /cc @Einhornhool @mguetschow
FYI: I am very ok with removing from and adding features to the lists |
|
||
* System Features | ||
- a preemptive, tickless scheduler with priorities | ||
- flexible memory management |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which memory management are we referring to here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably the one that does not force you to have a heap available but can use it if you need it -- my guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not add things that we are not sure what they mean ourself 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, as of now, the 2nd most prominent feature in the list.
I also don't know what is meant by that and I do agree that it should be dropped if nobody can explain it.
But maybe git praise
and ask the author first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line was introduced with the README.md 29.12.12 @784ef523900f3b0942c43099f57621b30f6cab73 - it is in it current form the the second oldest line in this file predated only by (due to a typo fix) -- if the git history wasn't manipulated.
Its original author does already know that this question exists.
nur
- a preemptive, tickless scheduler with priorities
und - IPv6
sind älter (und nicht leer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i did not misread tlsf-malloc replaces malloc if that pkg is used, so it has not to used explicitly -- ( the pool needs to be before the first allocation-call happens)
According to the initial paper it has:
- a lower complexity class O(1) than first fit alloc (O(length of free list)) (I am assuming embedded libc take the easy route and did not check)
- pooling is also an advantage (more safe) in that the malloc fails (at least that can be handled) rather then letting the heap run into the main stack
- can work with multiple memory regions ( external memory would just add to the pool)
- the low complexity class seems neither be achived by allways beeing cpu hungry nor by wasting lots of memory for management or letting it unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the malloc fails (at least that can be handled) rather then letting the heap run into the main stack
that's what any sane malloc implementation would do, tests/sys/malloc
will rely on that
- can work with multiple memory regions
I think this is a pretty niche feature, no need to advertise it so prominently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason to add TLSF in the first place was its real-time capability (because of O(1)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's --fail out of mem and not crash the stack-- what any sane malloc implementation would do,
tests/sys/malloc
will rely on that
I never checked where riot places the main/start stack in relation to the heap and it has a more sane placement than i thought - somehow had the default stackplacement of avrlibc in my head(endless stack and endless heap until they crash): [data bss heap> empty ram <stack]
, seems link the isr_stack might be less lucky sometimes but then the _eheap is adjusted.
thank you for that hint.
that only leaves the O(1) thing for tlsf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think allowing the allocator to be modified is quiet flexible but maybe this is not our feature but one of new/piolib(c)
- flexible memory management |
just to ensure that we are all on the same page: we do want a README that is not concise, and we want to present a long list of features in the README before talking about how to get started and get involved with RIOT? |
I see the problem with sprawling Readme files, but I think this is not the case here yet. Structure helps and this PR improves on that. Related Projects should be moved further down though. |
even though this was kicked off in that "Related Projects"-PR. I would like to keep this topic out of this PR there are other current PR that target this -- so please keep this out |
I already posted that in Matrix but it actually belongs here: |
And yes, I know that this post somewhat contradicts my previous comments. ;-) |
resolved some conversation (mostly just accepting) i still think this adds nice structure -- but I also see that this adds noise even though all these are features some off them are probably less finished (GNSS, PSA and TCP) or less noteworthy (everyone claims flexible memory management) shall some be removed? and lets not forget to look at how github renders this and not only on the amount of red and green lines |
Contribution description
Inspired by comments in #21052 especially #21052 (comment) this tries to improve Riots Feature List
Testing procedure
read kfessel:p-improve-feature-list
read kfessel:p-improve-feature-list:README.md
Issues/PRs references
#21052
at the same time ( for future reference)
#21061
#21063