Skip to content

Instantly share code, notes, and snippets.

@geky
Last active April 26, 2017 11:12
Show Gist options
  • Save geky/7eee6937f3d55552d4e365bf02de114f to your computer and use it in GitHub Desktop.
Save geky/7eee6937f3d55552d4e365bf02de114f to your computer and use it in GitHub Desktop.
////// mbed poll //////
// poll for mbed FileHandles
int mbed_poll(FileHandle fhs[], unsigned nfhs, int timeout) {
Timer timer; // set timer to keep spurious wakeups from keeping us from exiting
if (timeout >= 0) {
timer.start();
}
// register semaphore release as sigio callback on all filehandles
Semaphore sema;
for (int i = 0; i < nfhs; i++) {
fhs[i].sigio(callback(&sema, &Semaphore::release));
}
while (true) {
// check if any filehandles have pending events
int count = 0;
for (int i = 0; i < nfhs; i++) {
if (fhs[i].poll()) {
count += 1;
}
}
if (count) {
return count;
}
// return if time has expired
if (timeout >= 0 && timer.read_ms() > timeout) {
return 0;
}
// wait for events
if (timeout >= 0) {
sema.wait(timeout - timer.read_ms());
} else {
sema.wait(osWaitForever);
}
}
}
////// FileHandle defaults //////
class FileHandle {
public:
// By default filehandle is always ready for read/write
virtual uint16_t poll(uint16_t events) {
return POLLIN | POLLOUT;
}
// By default sigio callbacks are called every second
virtual void sigio(Callback<void()> cb) {
if (cb) {
if (!_sigio_ticker) {
_sigio_ticker = new Ticker;
}
_sigio_ticker->attach_ms(cb, 1000);
} else {
delete _sigio_ticker;
_sigio_ticker = NULL;
}
}
// Make sure ticker is cleaned up
virtual ~FileHandle() {
sigio(NULL);
}
}
@kjbracey
Copy link

kjbracey commented Apr 7, 2017

Thoughts:

sigio/attach is intended for public use - we can't hijack it for poll, it's a parallel public mechanism to the poll. I would say we keep the poll_change, but make it non-member. I think I made it a member purely so only things-derived-from-FileHandle could call it, but seems pointless.

You've lost all the detailed flag handling somehow - there was quite a bit of fine detail there.

This won't work if two threads use poll at once. You need to wake every waiting thread, so call the semaphore at least N times where N is the number of threads waiting on poll. Even then, that wouldn't work reliably - you might wake one thread twice. I'm not going to dig any further on that point, as this is exactly the hole digging I was talking about not wanting to do in my PR comment.

If you keep going I think eventually you'll reconstruct a something which is equivalent to a wait queue or condition variable, so I think it'll be faster and easier to start immediately with Russ's condition variable implementation.

@kjbracey
Copy link

kjbracey commented Apr 7, 2017

Oh, and don't think the default sigio implementation is appropriate, as the default is files, and files don't block - they're always readable and writable, so SIGIO doesn't happen.

Would be a reasonable default for sockets, but I'd go for 100ms.

@geky
Copy link
Author

geky commented Apr 7, 2017

You've lost all the detailed flag handling somehow - there was quite a bit of fine detail there.

Yeah, I may have skimmed over that too much, I was focusing on the thready bits.

This won't work if two threads use poll at once. You need to wake every waiting thread, so call the semaphore at least N times where N is the number of threads waiting on poll. Even then, that wouldn't work reliably - you might wake one thread twice. I'm not going to dig any further on that point, as this is exactly the hole digging I was talking about not wanting to do in my PR comment.

If you keep going I think eventually you'll reconstruct a something which is equivalent to a wait queue or condition variable, so I think it'll be faster and easier to start immediately with Russ's condition variable implementation.

Good point, you've convinced me. It does sound like the wait/yield solution is ok for now.

Oh, and don't think the default sigio implementation is appropriate, as the default is files, and files don't block - they're always readable and writable, so SIGIO doesn't happen.

Would be a reasonable default for sockets, but I'd go for 100ms.

Good point. Although should sockets just be implemented as blocking only at that point.


sigio/attach is intended for public use - we can't hijack it for poll, it's a parallel public mechanism to the poll. I would say we keep the poll_change, but make it non-member. I think I made it a member purely so only things-derived-from-FileHandle could call it, but seems pointless.

Hmm

Honestly overriding the existing sigio seems fine to me. The sigio function is a building block for operations like poll, and if you're using the poll function you wouldn't need to use sigio.

What I don't want to see is a backdoor for our specific implementation of poll. If we want to avoid overwriting the existing sigio I think we sould provide some way to append sigio handler (maybe get the existing handler). But this approach seems like it may not be worth it to me.

@kjbracey
Copy link

kjbracey commented Apr 26, 2017

Although should sockets just be implemented as blocking only at that point.

Sockets' default behaviour isn't the same as disk files' default behaviour. I don't think we'll ever have blocking-only sockets - we require non-blocking operation for them.

if you're using the poll function you wouldn't need to use sigio.

Disagree - you could be being event-driven, and using sigio to prompt non-blocking poll() calls. Which is what the PPP data pump is doing, if I recall correctly.

I think poll() is very much a core part of the system - to make it work across a variety of different file handle providers, there has to be this common agreement that they all get together and call that central point. It's not as if there will or can be multiple poll() implementations in a system - there can only be one.

There's not a lot of point in having a dynamic callback in every object and making them all point to that central point. Part of the API of being a FileHandle is that you work in mbed::poll(), as well as issuing SIGIO to users.

(I anticipate that maybe in future we define a subclass FileHandleWithoutPoll which gives you sigio(), but not the poll(). Abstracted Sockets could be that.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment