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

Improve scheduler #506

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Improve scheduler #506

wants to merge 5 commits into from

Conversation

pyhys
Copy link
Collaborator

@pyhys pyhys commented Jun 3, 2022

No description provided.

@pyhys pyhys requested a review from olbjo June 3, 2022 16:39
Jonas Berg added 5 commits June 8, 2022 07:48
Copy link
Collaborator

@olbjo olbjo left a comment

Choose a reason for hiding this comment

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

I had a major comment on adding current time to the api. Think it though and comment on how you want to do it.

After that I can do some additional review of the scheduler internals


/**************** Scheduler *******************************/

volatile pf_scheduler_data_t scheduler_data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the name should be "scheduler" adding a "_data" suffix does not add any value.
This is just an opinion.

os_mutex_t * scheduler_timeout_mutex;
uint32_t scheduler_tick_interval; /* microseconds */

/**************** Scheduler *******************************/
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think adding "/***************" style comments improves readability.
Possibly to highlight a group of attributes.
(Opinion)


/** Time between stack executions, in microseconds */
uint32_t tick_interval;
} pf_scheduler_data_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe pf_scheduler_t instead of pf_scheduler_data_t ?

void pf_scheduler_init (pnet_t * net, uint32_t tick_interval);
void pf_scheduler_init (
volatile pf_scheduler_data_t * scheduler_data,
uint32_t tick_interval);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a callback to get_current time should be registered at initialization. os_get_current_time_us() could be registered for normal execution and a mock for test.
This solution would mean less changes in the api and it reduces the complexity to the user of the API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point


if (net->scheduler_timeout_mutex == NULL)
if (scheduler_data->mutex == NULL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this check? In the init function I expected a memset 0 of the scheduler instance and initialization and and creation of various data entries.

scheduler_data,
ix_free,
scheduler_data->busylist_head))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Extra blank line?

/**
* Check if timeout \a a is scheduled before timeout \a b.
*
*
Copy link
Collaborator

Choose a reason for hiding this comment

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

extra blank line

@pyhys pyhys marked this pull request as draft January 17, 2023 10:10
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.

2 participants