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

Adding non-blocking function to ndn_app API #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

astralien3000
Copy link
Contributor

Using an other thread to execute ndn_app_run is not a good idea.
A solution is to add a function ndn_app_run_once that does the same, but returns when there is no msg in the queue.

@wentaoshang
Copy link
Contributor

I'm not sure about the intention for the new ndn_app_run_once function. The original ndn_app_run API tries to emulate the behavior of the event loop mechanism in JavaScript and Boost.ASIO (which are used by ndn-js and ndn-cxx libraries, respectively). The application simply starts the event loop by calling ndn_app_run then the API is responsible for executing the callback functions automatically based on the incoming messages.

I kind of understand the use case for the ndn_app_run_once function, which gives the programmer more control over when to process the messages. But this is breaking away from the event-driven programming model that all the other NDN client libraries are using right now.

@astralien3000
Copy link
Contributor Author

I just want to handle several event loops at the same time in the same thread.
Indeed, if I create an other thread to execute ndn_app_run, and I call ndn_app_register_prefix in the first thread, the system does not work.

@wentaoshang
Copy link
Contributor

I think by definition each thread should have only one event loop that runs forever within that thread. Having multiple loops in a single thread does not make sense unless you want to do manual priority-based scheduling among simultaneous events. Some more advanced systems provide such capability inside a single event loop using multiple event queues (such as the Grand Central Dispatch in macOS/iOS), but I don't think it's necessary for constrained platforms like RIOT.

@astralien3000
Copy link
Contributor Author

Ok, but because of this line, the thread that calls ndn_app_register_prefix receive the inter-thread messages, and not the one executing ndn_app_run.

I can't block the main thread because of user constraints. So that there is two solutions : either using non-blocking methods in the main thread, or making the library work independently from the threads.

@wentaoshang
Copy link
Contributor

the thread that calls ndn_app_register_prefix receive the inter-thread messages, and not the one executing ndn_app_run.

The model of ndn-riot is that application runs in a thread and interact with the NDN protocol stack, which is another thread. That's why ndn_app_register_prefix needs to send inter-thread message to the NDN thread to create a FIB entry (pointing to the app thread) in the NDN protocol stack.

I'm trying to understand your use case here: is the main thread actively taking user input so it can't be blocked? Is it possible to apply the event-driven programming model for your application? (BTW: you can always use ndn_app_schedule to schedule a future event within the app thread event loop.)

@astralien3000
Copy link
Contributor Author

My use-case is that I try to implement a ROS2 Middleware using ndn.
And the ROS2 interface handles 2 use-case :

@astralien3000
Copy link
Contributor Author

After further investigation, an other solution would be to send the pid of the thread running the ndn_app with the inter-thread messages' payload. It would not change the API. But in my opinion it would be harder to change.

@astralien3000
Copy link
Contributor Author

Basically, what would be perfect would be an interface like in ndn-cxx : Face::processEvents.
The timeout parameter enables a lot of things.

@astralien3000 astralien3000 reopened this Apr 18, 2018
@cawka
Copy link
Contributor

cawka commented May 23, 2018

Can you resolve the conflict and re-push?

@astralien3000
Copy link
Contributor Author

I will, but not now, I need to check if this code is not outdated. Also, I want to wait for this RIOT PR to be merged before adding this feature.

@astralien3000
Copy link
Contributor Author

PR updated, and rebased ! The diff is dirty, but I'm more confident with this version. Indeed the old one was modifying the behavior of ndn_app_run while this one should keep the same behavior. I tested this PR as a RIOT pkg, ndn-ping is working fine.

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