Feedback Thread: WPILib

[Greg McKaskle]

Spurred on by feedback scattered in other threads, I'd like to request detailed feedback on the control system. To help organize things, I've created three feedback threads, one for the HW elements that are used on the robot, one for the Dashboard and Driver Station, and one for WPILib.


Feedback Topic:
-----------------
WPILib


Tips on giving feedback:
---------------------------
Please be specific as to which elements are being commented on.

Not all teams use elements in the same way, so there is no need to argue that your value judgement for a component is the right one. Explain or justify your judgement so the expectations and context of use is clear.

While comparisons are a fine way to provide feedback, be sure to capture the context that is in your head. What did you expect it to do? Where did it fail and succeed? And then, tell how that compared to the other experience.

Please include tips on best-practices -- a good tool used poorly doesn't lead to a good experience, and the knowledge you can share may make a huge difference to someone else.

Once you've given the context please give your thoughts on improvements.

Greg McKaskle

[Radical Pi]

Some sort of flow limiter on the error messages that can be thrown out by WPILib would be nice. We run with a CAN bus (serial), so this is especially visible for us. There are 2 cases where I've specifically noted the problem.
1) Camera hasn't booted yet. We always wait for the camrea images to start updating on the dashboard before running the robot because the jags intermittently disconnect until the camrea is ready. According to the console there are a ton of E_CONNREFUSED errors being thrown that seem to delay the robot code enough to cause the jags to miss heartbeats and also cause serious control lag.
2) Disconnect enough jags. Basically the same symptoms as the one above. If enough jags lose power (intentionally or unintentionally) the errors thrown to the console from the attempted messages cause the same thing as above.

Also a small CAN library bug: Ever since updating to v87 (or whichever fixed the ID reset) the fwversion shown on the console when the CANJaguar class is initialized is always 0

[Dave Scheck]

One thing that comes to mind is that the Camera task (at least on the C++ side) doesn't clean up nicely when its destructor is called. This is only really an issue when running out of RAM, since a reset is required to run out of flash anyways. We hit this frequently enough during debug that we had a special compiler flag to disable the camera task when we weren't using it. If we were to try to debug in RAM while the camera task was running, we'd get an error (sorry I don't remember the specific one) about something already being defined (I believe it was in the video server).

Another issue that we saw was with the DPad class. It was reported here, but I'll make note of it here for reference.

In general though, WPILib is a great foundation and we use it extensively. I will post with more ideas at another time.

[taichichuan]

One of the things that I found difficult to work with in WPILib was the approach for creating and maintaining threads. There is a good model for this in C++ in the form of the Open Source Thread Building Blocks put out by Intel. There is an O'Reilly book on the TBBs as well. So, it's open source *and* documented :-).

Another issue that I've encountered is that within the Teleop control loop, the Wait() is implemented as a taskDelay. This causes the teleop loop task to go to sleep forcing it to lose contact with the operator. Normally, in a real VxWorks system, we'd have a thread that would be blocked on a binary semaphore and a watchdog timer (via wdCreate/wdStart) that would give the semaphore after the expiration of the timer. Or, create a thread and put it into a timed semTake call. This really relates to the way the code is designed for operating in teleop mode.

Another alternative for this would be to use the semaphore event mechanism to effectively deliver a signal (non real-time hack alert ;-), to allow the teleop loop to continue to run without putting the task to sleep. The concern that I have is that the kids don't realize the effect of using the Wait() call on their code and this makes the cRIO appear to not be fast enough.

HTH,

Mike

[FRC4ME]

I liked the functionality that WPILib provides, but I don't think it does a very good job of utilizing OOP principles. As an example: many classes inherit from SensorBase, and SensorBase then has specific methods for each of its child classes. That's not really how inheritance is supposed to work.

A good example of OOP in WPILib is the PIDSource interface. Unfortunately, this seems to have been added on as a side feature, and doesn't really integrate well with the rest of the library. It is, in my opinion, too specific; for something to be PID-compatible, someone has to have written a pidGet() or pidSet() for it. More often than not, pidGet() simply calls get(), and it would have been nicer to extract the getters and setters themselves into interfaces. In my team's code, we have four interfaces: AnalogIn, AnalogOut, DigitalIn, and DigitalOut. These have get(), set(), isOn(), and setOn() methods, respectively. Our PID class, therefore, automatically works with any input or output of the correct form, without us having to write special pidGet() or pidSet() methods in each class we want to be PID-compatible. I would like to have seen WPILib utilize these time-saving OOP features more often.

One of my favorite features of WPILib is the FPGA interfaces. I wish WPI would expand these and make them more configurable. For example, the gyro accumulators are great, but there are only two of them, and you can't choose which two analog inputs to attach those to. Moreover, the accumulators don't give us much control in the way of input filtering (thankfully there is a deadband function).

Finally, thank you so much for the new vision library! You really got it right this time; we downloaded the sample code to our robot and had accurate tracking of the target on the first try. Awesome!

[Greg McKaskle]

Since this is primarily a feedback thread, I'm not going to try to respond to most of the posts, but I want to ask a few followup questions on some suggestions and answer a few things.

Error rate:
The error console is a new thing that went in late in C++ and Java. It seems like a good idea, but uncovers code such as the camera initialization which shouldn't immediately retry on error. In fact, in LV the camera code spawns and runs in parallel and sleeps when it receives the "not ready" error that occurs during startup. Meanwhile, other error storms are easy to cause and currently lag the DS or cRIO or a bit of both. Some additional filters that count duplicates or otherwise avoid the storm is clearly an improvement.

On the D-pad:
The current protocol limits the number of axis and forces POV sticks or dpads to look like a pair of axis. Perhaps the Joystick stuff should be updated to send more info, and perhaps support feedback. Other feedback in this area?

Thread Wait():
I'm not nearly as familiar with the C++ implementation, but it sounds like this is a "teaching opportunity". If I understand your suggestions it is to spawn a specific task that is suspended so that teleop can continue. Either teleop or the independent task will set the resume time for the independent thread. That seems more like a useful pattern, and not an issue with wait(). I assume the default framework doesn't call wait() with a large value, that would not be a good thing to show. It sounds like there needs to be some examples showing how to accomplish asynchronous tasks and teaching the tradeoffs between that and wait() in a single thread.

Vision:
The NI-Vision IMAQdX driver now supports Axis cameras, and presumably it is better than the simplistic TCP stuff that was put into WPILib. We intend to evaluate that to see if it deals with the buffer allocation and buffer copies better than the simple implementations.

Greg McKaskle

[byteit101]

I have to agree strongly with FRC4ME, the OOP is poorly implemented. When I look at the autocomplete for a digital input, I don't want to see stuff not related to a digital input.
I also don't like the error classes, aside from only being used in the PCVideoServer, they flood the autocomplete with unnecessary items. If all classes inherit from ErrorBase, it should have two methods: HasError() and GetError(), and GetError would return a object that can be used to set, clear, and get the error
Two alternate ideas for the PID classes would be to have every class implement a sensor interface that has a Get(), and then you could pass it to the PID stuff, or have a sort of function pointer that points to the method and/or objects to call on.
A better OOP sensor class inheritance would be nice, as well as a Get() and Set(value) for all sensors.
Some classes are somewhat confusing in name to a novice: AnalogChannel, AnalogModule, AnalogTrigger, AnalogTriggerOutput, etc...

One thing that would be really nice would be a cleanup of the public members. For instance, the DigitalInput has tons of stuff you don't really need:
CancelInterrupts (Whoop-de-do!)
CheckAnalogChannel (This is not analog. I don't care)
CheckAnalogModule (This is not analog. I don't care)
CheckDigitalChannel (Potentially for debugging, what does it check? the module is present? nope, the channel # is valid. how confusing)
CheckDigitalModule (Potentially for debugging, what does it check? the module is present? nope, the channel # is valid. how confusing)
CheckPWMChannel (confusing and unrelated)
CheckPWMModdule (confusing and unrelated)
CheckRelayChannel (confusing and unrelated)
CheckRelayModule (confusing and unrelated)
CheckSolenoidChannel (confusing and unrelated)
CheckSolenoidModule (confusing and unrelated)
ClearError (Not useful, I don't see an error ever being set in the code)
DeleleSingletons (what singletons?)
DisableInterrupts (Wow! The first potentially useful and non confusing method! But you do need to know what Interrupts are)
EnableInterrupts (Wow! another!)
Get (And another! This one will be use by most people, and is not confusing in any way!)
GetAnalogTrggerForRouting( Routing? whats that? why do I care? and why is it an analogtrigger when this is a digitalinput)
GetChannel (Didn't I set the channel? did it change?)
GetChannelForRouting( Routing? whats that? why do I care?)
GetDefaultAnalogModule (I don't need this, and its unrelated to digital inputs)
GetDefaultDigitalModule (Can't I just use the one item constructor?)
GetDefaultSolenoidModule (I don't need this, and its unrelated to digital inputs)
GetError (Do we have an Error? If the code set errors, yes, but otherwise, this is unnecessary)
GetGlobalError (A global error on a non-global object? what is this madness?)
GetModuleForRouting (Why so much rooting? is roto-rooter here?)
ReadInterruptTimestamp (Why do I care? Isn't WPILib supposed to be a higer level framework? do I have to manually compare timestamps?)
RequestInterrupts (I'm taking a nap until a useful function come up, so Interrupt my nap when one does)
SetDefaultAnalogModule (Its a DigitalInput, not Analog!)
SetDefaultDigitalModule (Useful for pointers if you like making work, but you could just use the two arg constructor)
SetDefaultSolenoidModule (Solenoid !=(in your wildest dreams) DigitalInput)
SetError (I can cause errors, he he he!)
SetUpSourceEdge(source edge. ok. sure (smile and nod))
WaitForInterrupt(A useful function if you know what interrupts are! state-changers, Its a final useful function!)

A better DigitalInput: (dashes represent members of the object the function above returns)
Disable
Enable
Get
GetError
-IsFatal
-Set
-Get
-Clear
-...
GetInterruptManager
-Cancel
-Request
-Enable
-Disable
-Wait
-...
HasError
WaitForStateChange

Also, the C interfaces are kind of strange, I don't really think we need them (also given the fact that I have not heard of any teams using C this year)
Namespacing would be nice, although possibly confusing to new users

Better documentation and help would be very nice. And along this line, a better commented default SimpleTemplate would also be helpful

It would be great if you fixed this TODO as well

/**
* @todo If this is going to last until release, it needs a better name.
*/
class SimpleRobot

[FRC4ME]

Originally Posted by byteit101

...

Good post. What WPI seems to have done with SensorBase is taken all of the low-level code that the user doesn't have to deal with, put it in one (or a few) base classes, and then had everything else access that code through inheritance. But that's not the purpose of inheritance, as evidenced by your post: all of that low-level code becomes as visible to the user as anything else!

At the very least, many of those methods should be made protected rather than public.

My team's DigitalInput class (which wraps around WPI's) has three public methods:

isOn() -- returns the state of the input
setReversed() -- allows us to flip the result so that it makes sense ("on" is true) based on the type of circuit connected to the input
isReversed() -- by convention, we provide a getter wherever we provide a setter

We never found a need to use the error handling and interrupt manipulation methods. But if we ever do, we will implement those functions as separate classes, as you suggest.

Of course, WPI needs a lot more code than this, but it shouldn't be visible to the user.

[Nadav Zingerman]

I haven't seen any LabVIEW feedback yet, so I guess I'll be the first.

• The RefNum Registry - I think that this is a bad idea. While it does simplify development by reducing the number of advanced concepts programmers need to know (TypeDefs mostly), it also violates key programming best-practices, such as not using globals unnecessarily. It also discourages a logical grouping of RefNums, such as putting all RefNums for a sub-system in the same cluster.
• Many VIs in the library are not sufficiently documneted (are they even ready for use?) such as the DMA and Interrupt VIs.
• It would be nice if there were support for additional features of joysticks and gamepads, such as tactile feedback (many gamepads have a vibration feature).
• Beta screen shots showed an "Experiment Framework" in addition to the Robot Framework. It would be nice it this were made available again (I'm assuming this is something similar to 2009's Basic Framework) for testing sensors and the like. Right now we open example files and modify them for this purpose.

I don't know where bug reports belong, so I'll report here:

• The DBL Array version of the polymorphic PID block does not work out of the box. It seems to be looking for some DLL. A workaround is to use the LabVIEW implementation by setting the "PIDimplementation" symbol to "G", but this probably hurts performance, and regerdless, not many teams will know to do this.
• The PID Autotuning VI is broken and fails at the autotuning proccess, also searching for a missing file.
• The PID palette exist twice. Once as in the main palette list and once under Control Design & Simulation. The latter contains more functions, some of them quite useful, so it's a shame to have them hidden away in a submenu. Besides, duplication is evil.

Overall, great job with WPILib

[Kingofl337]

While this isn't part of WPILib yet, the CANJaguar code, more specifically "BlackJaguar.out" crashes upon momentary disconnect from the network, either due to noise or a connection issue. Most communications protocols can handle a brief disconnect.

[jhersh]

Originally Posted by Kingofl337

While this isn't part of WPILib yet, the CANJaguar code, more specifically "BlackJaguar.out" crashes upon momentary disconnect from the network, either due to noise or a connection issue. Most communications protocols can handle a brief disconnect.

Please file a bug report at FIRST Forge so this can be addressed. Please include as much detail as you can.

Thanks,
-Joe

[jhersh]

Originally Posted by Nadav Zingerman

I don't know where bug reports belong, so I'll report here:

Bug reports for WPILib can be filed as trackers at FIRST Forge for all 3 languages.

Thanks,
-Joe

[FRC4ME]

Originally Posted by Kingofl337

While this isn't part of WPILib yet, the CANJaguar code, more specifically "BlackJaguar.out" crashes upon momentary disconnect from the network, either due to noise or a connection issue. Most communications protocols can handle a brief disconnect.

We experienced the same problem. In the end we had to stop using CAN on the competition bot, because the entire drive system thread would crash if it lost a single packet. With the heavy vibrations and impacts of competition shaking the connectors around in the CAN ports, this happens sometimes.

[jhersh]

Originally Posted by FRC4ME

We experienced the same problem. In the end we had to stop using CAN on the competition bot, because the entire drive system thread would crash if it lost a single packet. With the heavy vibrations and impacts of competition shaking the connectors around in the CAN ports, this happens sometimes.

I hear you. I intend to make it a lot more forgiving before next season. However, I haven't seen any "crashes"... can you explain in detail what you are seeing by creating a tracker over at FIRST Forge?

Thanks,
-Joe

[Radical Pi]

Originally Posted by FRC4ME

We experienced the same problem. In the end we had to stop using CAN on the competition bot, because the entire drive system thread would crash if it lost a single packet. With the heavy vibrations and impacts of competition shaking the connectors around in the CAN ports, this happens sometimes.

I've seen nothing like this. On our bot we were running without a jag for a while (thanks to the ID reset bug) while the code was still trying to communicate with it which would surely drop packets. Using C++ w/ black jag bridge.