-
Notifications
You must be signed in to change notification settings - Fork 11
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
Make C library overhead configurable #12
Conversation
|
The only issue with this would be - we would force our users to use C++20. It may be bad, as |
Not if we match the range of the random values returned by this function to match the range of its return type. In my example above I named it getPseudoRandomByte with the return type being
I meant a freestanding definition anywhere else in your program (not in this library): void operator delete(void*) noexcept { std::abort(); } |
I can see that. My objection to that is as follows: right now in 3 spaces (1 in serial and 2 in CAN) where random numbers are needed, |
Yes, 8 bits of entropy is expected to be enough for these. |
I'll do it shortly. Should we provide sample implementations with |
Maybe but I am not sure where. Do you want to put it in the README? We can't put it in the headers because that would require more preprocessing. |
I see two possible ways:
These files would be just a few lines of code, I assume: // kocherga_random_libc.hpp
#pragma once
#include "kocherga.hpp"
auto getRandomByte() -> uint8_t{
return std::rand() *std::numeric_limits<uint8_t>::max() / RAND_MAX;
} But at the same time for |
That's a lot of work for such a trivial feature. Let us add this one line to the README and call it a day: auto getRandomByte() { return static_cast<std::uint8_t>(std::rand() * std::numeric_limits<std::uint8_t>::max() / RAND_MAX); } |
91494da
to
f5aab3e
Compare
I think I've catched all things in this single commit. |
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.
Please reformat the sources using clang-format, put back the const qualifiers that you accidentally removed, and move section "Configuring Kochergá" up so that it is just under "API usage". Reformat the README such that the lines do not exceed 120 characters in length.
Thanks. I am going to get back to this after #13 is in. |
@asmfreak please merge master into your branch (or rebase, it doesn't matter because we're going to squash your commits) and make sure the CI is green. |
* assert may be implemented in a different way than libc. * std::rand can lead to unnecessary overheads from libc. * virtual destructors require an operator delete to be present Signed-off-by: delphi <cpp.create@gmail.com>
Signed-off-by: delphi <cpp.create@gmail.com>
Signed-off-by: delphi <cpp.create@gmail.com>
Signed-off-by: delphi <cpp.create@gmail.com>
@pavel-kirienko |
5764f3d
to
fbe4c00
Compare
Thanks Pavel. I had to introduce a couple of final adjustments but there is nothing major. Our robotic overlords will have this merged once the tests have passed. |
The standard C library and can produce a lot of unneeded noise in the firmware if it's not used.
This PR provides a fix for this problem.
There are several problems with standard C functions:
assert
may be implemented in a different way than libc.std::rand
can lead to unnecessary overheads from libc.operator delete
to be present (requires further investigation)assert
A commit is added to replace assert macro with a namespaced one -
kocherga_assert
, which can be defined to a user-specified code.std::rand
A commit will be added to make this configurable in
UAVCAN/serial
module.virtual destructors require
operator delete
See this question on why it may relevant. A change is proposed using this answer from the same post.
Since the solution above requires С++20, we should stick to a simpler solution - removing
virtual
from base classes. The virtual destructors aren't needed for Kocherga - all virtual classes are managed by the user and in the most cases user does not need to destroy derived objects at all. In the rare event destructon is actually needed, destruction through a base class pointer (which virtual destructors are needed for) must not be used if virtual destructors are disabled.In the commited version virtual destructors continue to be enabled by default.