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

Replace dumb_timer_queue with new task_timer #278

Merged
merged 5 commits into from
Nov 21, 2021
Merged

Conversation

luca-schlecker
Copy link
Collaborator

@luca-schlecker luca-schlecker commented Nov 19, 2021

Signed-off-by: Luca Schlecker [email protected]

It fixes #264 and lays the ground for #273 and #257.

This PR completely substitutes dumb_timer_queue for a new class named task_timer.
The advantages include:

  • Ability to use a custom timeout for each task (function call)
  • Doesn't use a static tick delay shared between all instances.
  • Moved io_service logic into this class where it in my opinion should belong. (Moved from http_server.h)
  • Unit test for task_timer

fixes #264, lays ground for #273 and #257.

Signed-off-by: Luca Schlecker <[email protected]>
@luca-schlecker luca-schlecker marked this pull request as draft November 19, 2021 19:53
@luca-schlecker luca-schlecker marked this pull request as ready for review November 20, 2021 16:06
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Functionally I don't see anything wrong. Though it might be a good idea to inline methods like get_default_timeout() or set_default_timeout().
Other than that I do have a couple things to point out. The first is that GCC gives a reorder warning when compiling, I'm guessing this is because of where timeout is placed in http_server.h.

The second is purely visual, and is about the comment style, since crow uses the following style (without the need for @brief):

/// Brief Description Here
///
/// Loooooong Description

And \ instead of @ for commands.

Lastly looking at this image:
image
\return might not work as you intended (I'm not 100% sure, it seems weird that the return type is there and there's a returns part). And there are a few places where a line break isn't showing (probably requires a <br>).

I'm sorry if any of the points are wrong or not explained well enough. Besides them the code seems quite good and running the tests and examples it seems to work as intended.

@luca-schlecker
Copy link
Collaborator Author

Isn't a member function with its body defined inside the class implicitly an inline function?
"A function defined entirely inside a class/struct/union definition, whether it's a member function or a non-member friend function, is implicitly an inline function." (cppreference.com)
It looks like in the screenshot of Doxygen you've sent, these functions are already classified as inline, supporting the above point.

I've seen that warning too, I'll look into it.

I'll change the comment style to align with Crow's style.

The introduced `timeout_` variable was initialized in a different order than the member initializer list suggested.

Signed-off-by: Luca Schlecker <[email protected]>
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Everything seems good now. Great job!

P.S. If you want to force push your changes to get rid of the commits let me know, otherwise I guess we can merge this.

@luca-schlecker
Copy link
Collaborator Author

I think it should be fine. 🚀

@The-EDev The-EDev merged commit b1718be into master Nov 21, 2021
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.

Apps are not independent of each other due to dumb_timer_queue.
2 participants