Skip to content
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

parameters metadata generate as single static constexpr header #11318

Closed
wants to merge 6 commits into from

Conversation

dagar
Copy link
Member

@dagar dagar commented Jan 28, 2019

This PR is some minor parameter system cleanup that I'd like to get in a base before pursuing a few more interesting changes (like additional data types) and reducing the required flash.

  • update parameter generation to produce a static constexpr c++ header
  • introduce bitset (similar to std::bitset) for tracking active parameters
  • delete old param linkage

@dagar
Copy link
Member Author

dagar commented Jul 26, 2019

@julianoes @bkueng can I get an initial review here?

Verifying some of the specifics like the different indexing (eg param_for_used) is where this stalled last.

src/lib/parameters/parameters.cpp Outdated Show resolved Hide resolved
src/lib/parameters/parameters.cpp Outdated Show resolved Hide resolved
src/include/bitset.hpp Outdated Show resolved Hide resolved
src/include/bitset.hpp Outdated Show resolved Hide resolved
src/include/bitset.hpp Outdated Show resolved Hide resolved
src/include/bitset.hpp Outdated Show resolved Hide resolved
@dagar dagar force-pushed the pr-params_constexpr branch 2 times, most recently from fc3dbeb to 4c9a489 Compare August 4, 2019 14:40
if (param_index < 0) {
return;
if (handle_in_range(param)) {
// FIXME: this needs locking too
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix this by letting the bitset use the atomic class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented in 33a4f8b with new AtomicBitset.

julianoes
julianoes previously approved these changes Aug 5, 2019
@dagar
Copy link
Member Author

dagar commented Sep 29, 2019

Splitting the parameter type and volatile into a separate generated static constexpr array saves about 2 KB of flash.

@dagar dagar force-pushed the pr-params_constexpr branch from 522bceb to 6d2dfe2 Compare September 29, 2019 21:24
src/lib/parameters/parameters.cpp Show resolved Hide resolved
@@ -379,35 +343,19 @@ param_find_no_notification(const char *name)
unsigned
param_count()
{
return get_param_info_count();
return param_info_count;
Copy link
Contributor

Choose a reason for hiding this comment

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

param_count() can be const, right?


/* perform a binary search of the known parameters */

while (front <= last) {
middle = front + (last - front) / 2;
int ret = strcmp(name, param_info_base[middle].name);
int ret = strcmp(name, param_name(middle));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be using strncpy to be safe?

{
size_t total = 0;

for (const auto &x : _data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is x? Due to auto and the cryptic variable name I don't know what I'm looking at 🤔.

size_t total = 0;

for (const auto &x : _data) {
const uint32_t y = x.load();
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this atomic if you're doing a load in every loop iteration?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes the count() isn't thread safe, which is kind of lame overall, but only used in a single place for parameters.

TODO: review

@stale
Copy link

stale bot commented Dec 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Dec 30, 2019
@julianoes
Copy link
Contributor

@dagar would actually be nice to get this in, right?

@stale stale bot removed the stale label Jan 30, 2020
@dagar
Copy link
Member Author

dagar commented Jan 31, 2020

@dagar would actually be nice to get this in, right?

Yes definitely. I think last time we got hung up on the atomic bitset, the was only atomic per integer within the bitset.

@stale
Copy link

stale bot commented Apr 30, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale
Copy link

stale bot commented Aug 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Aug 8, 2020
@LorenzMeier
Copy link
Member

Closing this as stale.

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

Successfully merging this pull request may close these issues.

4 participants