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

Add check_deadline function. #38

Merged
merged 10 commits into from
Feb 13, 2022
Merged

Add check_deadline function. #38

merged 10 commits into from
Feb 13, 2022

Conversation

hokeun
Copy link
Member

@hokeun hokeun commented Feb 11, 2022

See lf-lang/lingua-franca#403 for more information.

Relevant PR: lf-lang/lingua-franca#960

@hokeun
Copy link
Member Author

hokeun commented Feb 12, 2022

Another thing is that user-facing API should be defined/prototyped in ctarget.h.

Thanks for the comments, Soroush. I hope I got your comments correctly in c97abe5.

Copy link
Contributor

@edwardalee edwardalee left a comment

Choose a reason for hiding this comment

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

Very elegant and simple! Let's merge this.

core/reactor.h Outdated Show resolved Hide resolved
core/reactor_common.c Outdated Show resolved Hide resolved
lib/ctarget.c Outdated Show resolved Hide resolved
include/ctarget.h Outdated Show resolved Hide resolved
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Very nice!

bool _lf_check_deadline(self_base_t* self) {
reaction_t* reaction = self->executing_reaction;
if (get_physical_time() > get_logical_time() + reaction->deadline) {
reaction->deadline_violation_handler(self);
Copy link
Contributor

@Soroosh129 Soroosh129 Feb 12, 2022

Choose a reason for hiding this comment

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

@edwardalee The deadline violation handler function is called here. Is this semantically okay?

Because of the way check_deadline() is supposed to work, the deadline handler could be called multiple times (if the caller doesn't terminate the reaction (e.g., by using return) upon check_deadline() returning true, and calls check_deadline() again later). What if the deadline handler had some side effect, including changing a state variable and setting to an output port?
This semantic seems a bit strange to me. Depending on the body of the reaction, the deadline violation handler could be called multiple times within the same tag. Even stranger, the number of times the deadline violation handler is called depends on many external factors, like the load on the system.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we decided to do it this way to ensure that the deadline violation handler is called as soon as possible. We could alternatively put it on the reaction queue, but then it is possible that other reactions will be invoked first. Also, the main reaction may need to do some cleanup after the deadline violation is detected. I don't think we should delay invocation of the deadline handler until after that cleanup is finished, but if we don't call it here, that is what would happen.

I don't see anything fundamentally wrong with the deadline handler being called multiple times, if that is what the designer decides to do, although I do think this would be a strange choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, got it.

I think the way the API is documented on the website might be important to make sure programmers fully understand the ramifications of using check_deadline.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just merged the PR as I believe we reached an agreement. Please let me know if there's any further suggestion, I'll be happy to address that.

@hokeun hokeun merged commit 89dd926 into main Feb 13, 2022
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.

4 participants