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

feature/ND/230/Node-Event-Scheduler #5

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

ShaneTWilliams
Copy link
Member

Welcome to ghetto RTOS, folks

EventScheduler is responsible for calling callback functions at given intervals, essentially creating a system of (many) non-blocking delays. It's very heavily based on the Arduino timer we used last year.

The EventScheduler class owns an array of 50 Event objects, each of which has a delay, a count of how many times it needs to be called, a type, a state, the last time it was called, and a callback function. Through the EventScheduler class, you can set one of these Events to run a callback every X seconds. 50 is an arbitrary number which can be changed in the future.

EventScheduler has 3 methods that matter:

  • EventScheduler::callFunctionAfter calls the callback once after X seconds
  • EventScheduler::callFunctionEvery (version 1) calls the callback every X seconds
  • EventScheduler::callFunctionEvery (version 2) calls the callback every X seconds, but only up until the specified number of calls has been reached

Each of these returns an eventId_t ID which is used to identify that event, and can be handy when using the class's startEvent and stopEvent methods. Also, whenever you call one of those 3 methods, the class automatically assigns you an unused event object, and therefore an unused event ID.

Every loop in the main, you NEED to call the EventScheduler::updateEvents function. This is what checks every event to see if its delay has passed. If so, the event's callback is called. If the event has been run its maximum number of times, its Event::cleanup function is called, and it resets all its properties and becomes ready to be assigned again.

There's also unit tests under OnPod/Node/Test/EventScheduler. Pls review them too.

P.S. - the Event class definitely could have been been made a struct, but the current solution is super clean and leaves room for the future if needed.

Copy link
Member

@colton-smith colton-smith left a comment

Choose a reason for hiding this comment

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

Loved it.
Style-wise it's very clean, no complaints here.

Although I don't have much experience with event-driven code, I'm able to understand this which is a good sign in terms of readability. Code-wise the only thing I saw is the possibility of making the EventScheduler class a friend of the Event class, as I mentioned in the comment.

Testing is thorough - love to see it. I added a few ideas for test cases, the more boxes we check the better.

Great job fearless leader

Copy link
Collaborator

@andrewnash andrewnash left a comment

Choose a reason for hiding this comment

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

These comments are an amalgamation of me and Mark briefly looking at this together. Maybe we are missing something but unsure how this is non-blocking, pretty sure would need multiple threads to accomplish this? Should these events have some priority based on the time elapsed? Maybe some form of priority queue is better suited?

Copy link
Member

@Burke-Daniel Burke-Daniel left a comment

Choose a reason for hiding this comment

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

Looks great!

Style is clean and consistent, and I am a fan of the test function names, it makes it very explicit and easy to read.

Like Colton I don't have much experience with event driven code, however I understand what this code accomplishes, and anything I noticed has already been addessed by other's comments.

Great job!

@ShaneTWilliams
Copy link
Member Author

ShaneTWilliams commented Nov 22, 2019

Changed the main data structure to a map. It's just as fast (as a vector) when iterating and faster in finding available eventId, inserting and erasing.

Changed the Event class to a struct because it was only holding data anyways.

Added the tests suggested in the PR.

Moved the whole rig to OnPod/Common from Node/Common.

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.

5 participants