PIDController::CallCalculate() Not being called

[Joe Hershberger]

How are the handler functions for a non-watching interrupt manager done? Specifically, will the execution of the registered handler function ever be task-switched away from to execute, for example, the main robot task? Or is it something that cannot be interrupted?
If the former, does disable() do anything to make sure a call to the handler is not in progress and/or wait for any in-progress handler call to complete?

The registered handler runs in the context of the spawned task. As it exists right now, it is running at default priority. That means that it can task switch during the handler. The handler does however call taskSafe() before calling the handler, which means that if disable() is called while the handler is running, the disable call will block until the handler returns.

What I've seen so far is that the creation of a Notifier (hence a tAlarm and tInterruptManager being made and enabled) kicks off a vxWorks task called "tX" (where X is some number). Deleting the Notifier (and disabling and deleting the tAlarm and tInterruptManager) causes the tX task to go away.

The task for the handler is spawned in the call to tInterruptManager::enable() and the task is deleted in the call to tInterruptManager::disable().

Subsequently creating another Notifier (in the same main robot task or in a new task loaded via debug interface) will not cause another tX task to be created, and the Notifier never gets any calls to its handlers.

It certainly doesn't help that the destructor for Notifier doesn't set talarm and manager to NULL. If it were to run again, the objects are deleted, but not NULL so they aren't created when the constructor runs again. This is what you are seeing.

Another problem is that the manager and talarm objects are not reference counted so if you create 2 Notifiers, then delete one, the static variables are deleted and the remaining Notifier is broken.

Another problem is that the Notifier uses global variables. It looks like it would be far more appropriate for it to hold the tAlarm and tInterruptManager objects as static variables in the Notifier class.

The Semaphore also should be created on first use instead of statically initialized and should be a static member of the class.

Also m_head is defined in the class and never used, but timerQueueHead is defined globally and is used instead.

It seems this class needs some work.

[Mike Mahar]

I'm not trying to delete my Notifiers from my code. I'm using the terminate button on the debug window of Workbench. Does using workbench to terminate a task call any destructors for that class? Re loading my program resets all the static variables so the manager and talarm variables are NULL when I reload but the destructor of Notifier doesn't seem to have been called.

I'll check the destructor for my IterativeRobot and check to see that I'm deleting the PIDController object I created. I'll have to wait until I get there this evening.

[heydowns]

The registered handler runs in the context of the spawned task. As it exists right now, it is running at default priority. That means that it can task switch during the handler. The handler does however call taskSafe() before calling the handler, which means that if disable() is called while the handler is running, the disable call will block until the handler returns.

Thanks Joe - this is exactly what I needed to know.

The task for the handler is spawned in the call to tInterruptManager::enable() and the task is deleted in the call to tInterruptManager::disable().

ok -- this will be helpful when trying to debug where the problem with the handler task re-spawning lies.

It certainly doesn't help that the destructor for Notifier doesn't set talarm and manager to NULL. If it were to run again, the objects are deleted, but not NULL so they aren't created when the constructor runs again. This is what you are seeing.

Yes. This is the obvious issue with the Notifier destructor I spoke of.
I've already fixed this here, basically in the manner in which you describe (again, I'll post a patch tonight when I get back to our school).
But, fixing this didn't fix the overall issue of the task going away on tInterruptManager disable() and delete, and not being respawned when a new tInterruptManager is subsequently created and enabled().

So either I messed up fixing this or there's another issue here to be discovered. I'll be looking at it more tonight, armed with the knowledge you've given.

Another problem is that the manager and talarm objects are not reference counted so if you create 2 Notifiers, then delete one, the static variables are deleted and the remaining Notifier is broken.

Another problem is that the Notifier uses global variables. It looks like it would be far more appropriate for it to hold the tAlarm and tInterruptManager objects as static variables in the Notifier class.

The Semaphore also should be created on first use instead of statically initialized and should be a static member of the class.

Also m_head is defined in the class and never used, but timerQueueHead is defined globally and is used instead.

It seems this class needs some work.

I can clean these things up before I post the patch.
There's also some other things I've noticed that seem to be wrong with the queue management. I will post these as well.

Thanks, Joe, for your help.

[Joe Hershberger]

The handler does however call taskSafe() before calling the handler, which means that if disable() is called while the handler is running, the disable call will block until the handler returns.

I meant to say that:

The Interrupt Manager does however call taskSafe() before calling the handler, which means that if disable() is called while the handler is running, the disable call will block until the handler returns.

[heydowns]

ok sorry for the delay, but I think I finally figured this out.
There are two patches in the attached zip file. One corrects several issues in Notifier, including the one talked about in this thread. The other renames the m_semaphore variable to queueSemaphore to better reflect it's use. I separated them to make the behavior changes stand out without the clutter of the variable rename. Intended application order is behavior patch, then rename patch.

Joe was 100% right on; properly handling the deletion fixes it for a single program run. When I said before that it wasn't fixing it, I was mistaken -- I had a typo in my fix that I found upon further testing.

Other fixes in the patch:
1. Expiration times were being erroneously recalculated in UpdateAlarm(). If using more than one Notifier (PIDController) you could get incorrect sampling periods.
2. The queue was being mismanaged in InsertInQueue. When something was being added mid-list (not head), it got put in the wrong place. This could also cause incorrect sampling periods.
3. Based on Joe's description of how the interrupt manager's task works, there is a race condition between the handler task and the deletion of a Notifier. The object could be deleted while the handler task would be holding, and later dereferencing, a pointer to it. The patch should fix this by guarding that any in-progress Notifier::m_handler function call is completed prior to completing the Notifier destructor.
4. Non-periodic Notifier uses would result in unreliable settings for m_queued; patch sets this to false after the event is up for dispatch.
5. There was a spurious semTake() in DeleteFromQueue.

"Enhancements" (may or may not be desired for wider distribution):
1. Stop() now waits for any in-progress handler call to complete before returning.
2. Incorporated Joe's suggestions for moving the static globals into the class as static members.


All that said, we're using this here and having no more problems with deleting PIDControllers, making multiple PIDControllers, etc.

This does not fix Notifier when killing/reloading the program in the debug interface though. From what I can tell, with the default debug settings plus those mentioned in the FIRST C++ Programming Guide doc, the system does not seem to re-initialize static data (this can be seen with the console open, even in other areas of the code -- any other resources in use also error out with "unable to reuse allocated resource", you get an assert that RobotBase::m_instance != NULL, etc).

We did find a fix, however (hopefully OP is still reading - this is the part you want!). The fix is to go to the debug settings dialog (Run->Open Debug Dialog), click the "Downloads" tab, select the file in the list, click Edit Button, then make sure that "Download Even When File Not Modified" checkbox is checked. Then click "Advanced Options". In the new dialog, make sure that "do not unload the existing module" is UNchecked.
"OK" out of the settings dialogs, ensure that "reload" is checked in the main Debug Dialog", then click Apply.

Reboot robot, reload code, and you should be good.

Since applying the "Download Even When..." setting, the above mentioned errors and the Notifier not calling CallCalculate() issue have gone away for us when reloading code via the debug interface.
My suggestion would be that this get added to the Windriver Debug setup instructions

[virtuald]

I'll have to try this out. Thanks.

[tlewis]

We applied the patches supplied by heydowns in notifierfixes.zip and they do appear to work. I would like to ask that next time, please include the whole source file and don't use a .patch file. The problem we had was that we had updated to the new WPILib sources and weren't sure which version the patch file was produced from. The .patch file application didn't produce any errors, but it didn't appear to have patched the correct version -- there were a couple of places where the semGive() appeared immediately after the corresponding semTake(). I've fixed those and we have been able to reload successfully without rebooting. Anyway, here's what we've got for Notifier.h and Notifier.cpp. Have I missed anything?

[heydowns]

The .patch file application didn't produce any errors, but it didn't appear to have patched the correct version -- there were a couple of places where the semGive() appeared immediately after the corresponding semTake().

The take, give in immediate sequence is what was intended. The simple act of taking the handler semaphore after deleting the notifier from the queue makes the task wait for any in-progress handler dispatch to complete.

I would suggest that you change your version back -- the new code has the potential to deadlock if you call Notifier::Stop() or delete a notifier due to out of order semaphore acquisition. Just put the semTake right before the semGive in Stop() and ~Notifier.

The patches were against "Update 3" of WPILib. Here is the full copy of the version we've been using.

I'm curious - did you need to change your "Debug" settings as I described for things to work on Debug reload?

[Joe Hershberger]

The take, give in immediate sequence is what was intended. The simple act of taking the handler semaphore after deleting the notifier from the queue makes the task wait for any in-progress handler dispatch to complete.

I would suggest that you change your version back -- the new code has the potential to deadlock if you call Notifier::Stop() or delete a notifier due to out of order semaphore acquisition. Just put the semTake right before the semGive in Stop() and ~Notifier.

Just FYI, that is actually not needed. As I stated previously:

The Interrupt Manager does however call taskSafe() before calling the handler, which means that if disable() is called while the handler is running, the disable call will block until the handler returns.

This means that even if you call Stop() while a handler is running, that call to Stop() will block (wait) until the handler returns control to the Interrupt Manager, meaning there is no race.

FWIW, if there were a race, you would only have solved it by calling Stop() while holding the mutex (after semTake and before semGive).

[heydowns]

Just FYI, that is actually not needed. As I stated previously:

This means that even if you call Stop() while a handler is running, that call to Stop() will block (wait) until the handler returns control to the Interrupt Manager, meaning there is no race.

Hmm? Notifier::Stop() doesn't try to terminate the interrupt manager's task. It doesn't interact with the interrupt manager at all. It just stops the Notifier (by removing it from the alarm-generating event queue, possibly disabling the tAlarm object if the Notifier being stopped is the only one active).

You are right, there is no internal race in Notifier::Stop() - I don't think I ever said there was(?). The handlerSemaphore use in Notifier::Stop() simply ensures that any in-progress dispatch to that Notifier's handler completes before return from the Notifier::Stop() call. I explained that this was not a bug fix, rather an enhancement we found useful and offered to share, in my earlier post.

The race being fixed is in the Notifier destructor. If the Notifier being deleted is not the last Notifier in use, the interrupt manager isn't disabled, so taskSafe wouldn't come in to play (unless I misunderstand your previously explanations of how interrupt manager works). For deletion of any other than the last Notifier, the handlerSemaphore is the only thing to protect the interrupt manager task (once executing Notifier::ProcessQueue) from potentially following an invalid Notifier pointer.

FWIW, if there were a race, you would only have solved it by calling Stop() while holding the mutex (after semTake and before semGive).

No race in Stop() -- I think we're in complete agreement here