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 stop program functionality #6

Merged
merged 4 commits into from
Feb 3, 2024

Conversation

tohlh
Copy link
Contributor

@tohlh tohlh commented Jun 4, 2023

No description provided.

@tohlh tohlh requested a review from RichDom2185 June 4, 2023 12:16
@angelsl
Copy link
Collaborator

angelsl commented Jun 4, 2023

I'm a bit confused by what this is for. Sinter was intentionally run in a separate process (sinter_host) from Sling proper so that Sling could simply just kill sinter_host if it needs to stop the program. If this is to fix a bug where stopping the program from Source Academy doesn't work, I don't think this is the right fix. Sling already attempts to kill (i.e. sends SIGTERM to) sinter_host when a stop command is received, so if stopping isn't working then there is a bug somewhere that should be fixed.

In any case, it's not clear how sinter_stop is going to be called. It has to be called from within the process in which Sinter is running. In the case of the EV3/Sling, that is sinter_host, which again already gets killed (or at least, should get killed) by Sling when it receives a stop command from the frontend.

If the intention is to call sinter_stop and thereby set running = false from a different thread (but I'm not sure how that thread is going to know when to call sinter_stop), you'll need to do this with proper memory ordering (running = false with release, and the load of sistate.running with acquire), otherwise this may not work as expected.

@angelsl
Copy link
Collaborator

angelsl commented Jun 4, 2023

@tohlh
Copy link
Contributor Author

tohlh commented Jun 4, 2023

Hi @angelsl.

Actually this change is made specifically to stop sinter from running when being used on Arduino, and sinter_stop will be called on the Arduino sketch. As Arduino is a microcontroller and it doesn't run Sinter as a process, so we figured this would be the only way to stop the running program.

Hope this can give more clarity, please do suggest if there is a better way to implement the above. Thank you!

@angelsl
Copy link
Collaborator

angelsl commented Jun 4, 2023

I guess it makes sense then. Does Arduino have multi-threading though? Or are you running some sort of periodic interrupt to check for messages from the network etc?

In either case, you should do the load/store with the right memory order. See https://en.cppreference.com/w/c/atomic/atomic_load and https://en.cppreference.com/w/c/atomic/atomic_store.

Alternatively you could make the running field volatile but it would be better if you use atomic_load/atomic_store instead.

@angelsl
Copy link
Collaborator

angelsl commented Jun 4, 2023

Actually I take it back, it might be better to just slap volatile on the running field since it's conceivable that a C environment that doesn't have multithreading might simply implement acquire/release semantics as load/stores.

@tohlh
Copy link
Contributor Author

tohlh commented Jun 4, 2023

Or are you running some sort of periodic interrupt to check for messages from the network etc?

Yes, I am using Timer5 library to check for messages periodically.

it might be better to just slap volatile on the running field

I have just made this change, thanks for the suggestion!

@RichDom2185 RichDom2185 merged commit 1cfc21f into source-academy:master Feb 3, 2024
8 checks passed
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.

3 participants