wpilibsuite / allwpilib Goto Github PK

I tried it tonight with a variety of different configurations, in RobotPy and using Java just to double check. When I create a CANTalon object with a device number that isn't connected, I expect the robot to inform me somehow, but there are no exceptions, no error messages, nothing.

I think the CANJaguar used to throw random exceptions at runtime if a Jaguar became disconnected, and I don't think that's a good behavior -- but now the pendulum has swung to the other extreme. I think reasonable expectations are:

At robot startup, if the device doesn't exist:

• Always report an error message to the DS
• Throw an exception if the FMS isn't present

If the device becomes disconnected after the object has been created:

• Always report a (rate-limited) warning message to the DS

2423 got bit by this a lot during the season. :(

It would seem that in the C++ version of the WPILib library, any call to AnalogInput::GetSampleRate() (wpilibc athena, 2017 Beta 1) causes a segmentation fault. It seems that this is due to the HAL_GetAnalogSampleRate(int32_t *) function (hal athena, 2017 Beta 1), specifically at line 139.

138 | double HAL_GetAnalogSampleRate(int32_t* status) {
139 |  uint32_t ticksPerConversion = analogInputSystem->readLoopTiming(status);
140 |  uint32_t ticksPerSample =
141 |      ticksPerConversion * getAnalogNumActiveChannels(status);
142 | return static_cast<double>(kTimebase) / static_cast<double>(ticksPerSample);
143 | }

The issue appears to be in the call to analogInputSystem->readLoopTiming(status), which is interesting as the backtrace does not enter the readLoopTiming call, stopping at HAL_GetAnalogSampleRate. Running gdb confirms this claim.

My initial assumption would be that this issue is due to the linkage to the ChipObject library. Please note that this is the 2017 Beta 1 library running on a 2016 RoboRIO image.

To dispel the obvious: This call is made after RobotBase->StartCompetition (called in periodic functions), and is also made only after at least 1 AnalogInput instance has been created.

Regards,
~Jaci

The current hardware test platform (used by Jenkins) is a little finicky because it uses mechanical components, and also does not cover several areas of the hardware such as I2C. Using a component such as an Arduino to measure PWM signal characteristics, drive analog inputs, interface to SPI and I2C, etc, may be one option to eliminating mechanical components, improving coverage, and making it easier for developers to replicate the test platform at their own locations.

This is only currently there for testing purposes, but makes it hard to test when we can't pass the returned string directly to open(). Also we should make that header public, along with the VISA headers. Also make it not take a resource handle since we can open multiples of those, so they don't need to be passed in.

In Java the first object you add to a SendableChooser is marked as default. In C++ this is not the case. What implementation should we go with?

https://github.com/wpilibsuite/allwpilib/blob/master/wpilibc/shared/src/SmartDashboard/SendableChooser.cpp
https://github.com/wpilibsuite/allwpilib/blob/master/wpilibj/src/shared/java/edu/wpi/first/wpilibj/smartdashboard/SendableChooser.java

The current LabVIEW dashboard (Beta 4) requires the source type to be either usb: or ip: and does not support cv:. To work around this, it's necessary to use a source type of usb: for OpenCV sources as well.

I believe that DriverStation.waitForData and DriverStation.isNewControlData should have the following guarantees associated with them (and it should be documented either way):

• waitForData will not return until new data is available
• when waitForData returns, isNewControlData will return true the first time that you call it after waitForData is called

Currently, the Java implementation does not appear to actually meet the second guarantee, as the locking related to these are separate. I haven't looked at C++ closely, but since #233 exists I'm sure it doesn't meet the guarantee either.

To fully meet the first guarantee, I think the Java implementation would need to swallow the InterruptedException. Perhaps this calls for waitForData and waitForDataInterruptibly (which would allow the InterruptedException to propagate).

The test bench will not test a Spike when #300 gets merged.

Using wpilibj beta 2, Joystick.getRawButton() always returns false for any buttons beyond button 8. Looking more closely, when pressing button 8 getRawButton() returns true not just for button 8 but for all remaining buttons present on the stick.

This appears to be caused by the JNI layer treating the buttons field as a byte rather than an int.

There are a fair number of documentation comments and general code comments that still date from the cRIO days. At some point, it would be nice to go through the library and make sure they are all up to date.

Too late to do this for 2017, so will do for 2018.

When a team selects an autonomous mode on the dashboard, then restarts (or starts) the robot, the robots default value for the sendable chooser overwrites the user selected value since it came in after the dashboard started. The desktop value should have priority so they can set preferences before the robot is finished booting. This seems to make sense unless someone can come up with a counter-example where the newer robot value should take priority over the users input at the dashboard.

I think the list of values is showing up because the dashboard layout is saved.

The core of the problem is that onTarget is calculated using the err buffer, which gets cleared when a new setpoint is set. This behavior is incorrect -- onTarget should not depend on the history of the setpoints (which is how error is calculated), but instead it should only depend on the history of the inputs as that's really the question a user is asking when calling onTarget: "is the input [history] close to where I want it to be?"

To that end, I propose that a buffer of inputs is kept in addition to a buffer of errors, and use the inputs to calculate onTarget instead. This buffer would only be cleared when the PIDController is disabled.

Currently all WPILib classes are in the global namespace. This is not good practice for a number of reasons (WPILib additions can conflict with existing user code, makes it easier to violate the one definition rule and end up with undefined behavior). All classes should instead be in a namespace (wpi and frc are reasonable options). A backwards compatibility shim can be provided (turned on by default) with something like the following in the headers and/or adding it to the Eclipse template:

#ifndef NAMESPACED_WPILIB
using namespace frc;
#endif

When ChipObject.h's headers are lexographically ordered per our style guide, compilation errors occur. This is caused by NI's headers in the FRC_FPGA_ChipObject folder not #including files they need, such as tSystem.h. There may be more files required, but tSystem.h caused the most errors and filled up my console backlog.

Currently, WPILIbC is being published to the WPILib Maven.
Unfortunately, this maven entry does not include any dependencies in the maven pom, nor any packaged with the zip. Given that WPILibC depends on NTCore, CSCore, WPIUtil and the HAL, While the HAL follows the same versioning scheme as WPILibC, NTCore, CSCore and WPIUtil are all separate projects with their own versions for each release of WPILibC.

The issue is that if a project is fetching a prior version of WPILibC from Maven, it can become challenging to determine what the correct version of the other libraries should be, as the latest available may have compatibility issues with prior versions of WPILibC.

Copy of https://usfirst.collab.net/sf/go/artf4112

C++ and Java will now just throw a Parameter Out Of Range exception error, which Isn't great. I'll go through and update these to give better error messages.

It might be the case where a user wants more then 8 1x or 2x encoders, however since by default since those always use counters, you can't have more then 8. With the move of encoders to the HAL, it should be possible to allocate 1x or 2x encoders to FPGA Encoder objects fairly easily, as long as the FPGA Encoder object can handle 1x or 2x decoding. Worth checking out. If that doesn't work, it might be worth printing an error message stating try switching to a 4x encoder, as those might be available.

All the gory details are here:

https://gist.github.com/phurley/7ab39f4fc4317b1107bb35573a5f6081

An issue regarding WPILib's maven publishing has recently come to my attention through this peculiar issue on GradleRIO.

In short, versions are being incorrectly published to maven. All new releases are being published under the same version 0.1.0-SNAPSHOT, with subversions for each release (and beta release). Because of this, anything that fetches from maven (gradle being a prime example) will only fetch once for its cache, but this cache will not invalidate with new releases as it sees no new versions published, and so the build version of a library can entirely depend on the system it's running on, breaking not only community forks and projects, but also can lead to an ambiguity in "it works on my machine", however for other members as well as CI services, the build breaks.

Compare this to a maven repository that is properly versioned, such as GradleRIO or even the development branch of WPI's repository, you can see how this becomes an issue.

The calculate function does an awful lot of stuff while holding the lock. It would be better if it copied the things it needed to local variables, and then performed the calculation. This would prevent the main robot thread from stalling.

Current C++ implementation is:

bool DriverStation::IsNewControlData() const {
    return m_newControlData.tryTake() == false;
}

This is correct IFF you are calling it a lot and never forget to call it. But really, it's just a bad way of doing this. The Java implementation is actually correct.

The threshold method incorrectly calls res.free() prior to returning res, so the returned BinaryImage is unusable. This was introduced in 3c4a1d9

Originally reported at https://www.chiefdelphi.com/forums/showthread.php?threadid=149111

It returns a raw calculation, does not compensate for wraparound (see code inside calculate() for correct implementation).

These should be integrated into the style guide eventually.

General

• C++ is intended to be fast, so implementation tradeoffs should be biased toward that

Type Names

• HAL should use stdint.h typedefs
• wpilibc
  • use int in user-facing interfaces instead of int32_t or uint32_t
  • all integer types shall be signed in external interfaces
  • only use unsigned types for internal bitwise arithmetic
• wpilibj
  • use int in user-facing interfaces instead of byte

Floating Point

• doubles should be used everywhere instead of floats
  • compiling with optimization turned on shows negligible performance impact compared to floats

Generated Files

• Since changes would be overwritten upon regeneration, generated files should not be modified under any circumstances, including for formatting changes. The generator for those files should be modified instead.
• The current list of generated files is represented by regular expressions in styleguide/task.py.

I'm not 100% sure what the original author intended for this method to be but I'm fairly confident that it doesn't do what it's documented to do.

https://github.com/wpilibsuite/allwpilib/blob/master/wpilibj/src/sim/java/edu/wpi/first/wpilibj/DriverStation.java#L135

Also, its missing the Thread.curentThread().interrupt() to reset the interrupt flag.

Right now, there is no resource allocation on CANTalon, so you can allocate the same ID with multiple objects. Is this something we would want to fix? It would be a breaking change, at least in java, since we would be throwing a new exception that we were not doing before.

Unzipping wpilibj/build/libs/wpilibj.jar reveals it contains two full copies of ntcore's NetworkTables.jar, in particular two copies of Linux/arm/libntcore.so (a 700K file zipped to 225K). This duplication unfortunately propagates into FRCUserProgram.jar builds, resulting in a ~225K larger jar copied to the robot.

I propose that the vision libraries in WPILIb move from NI Vision to OpenCV for a number of reasons:

• In the real world of college, research, and industry OpenCV is the standard for computer vision. This is what our students will likely eventually be learning
• OpenCV is well supported, well documented, lots of samples, videos and books available for many computer languages.
• While I don't want to mandate this, any team that chooses to use a co-processor will certainly be using OpenCV as their vision library and often NetworkTables for communications to the robot. Although we might not provide a packaged solution for common co-processors, there is enough community activity in this area to make it much easier than it's been in the past.
• GRIP has increased the use of and awareness of OpenCV and made it more approachable.

What problems should we solve:

• writing OpenCV robot programs should be as easy as using anything else in WPILib. It should work with both C++ and Java for some version of OpenCV (could be version 2 or 3). The libraries, build files, scripts, and anything else required should be packaged with the libraries/plugins.
• Using cameras, both USB and ethernet cameras should just work. The camera settings should be available from the program so that you can, at least, set frame rate, image size and compression, and exposure.
• You should be able to display annotated images on the Driver Station computer, probably through a SmartDashboard plugin. Minimally, the raw would be displayed, but one could use the openCV annotation functions to draw on the images depicting counters, bounding boxes, etc.
• A team just wanting to stream the camera (from the robot) output on their Driver Station, again probably through the SmartDashboard, should be able to do that with minimal effort and minimal performance degradation to the roboRIO robot program.
• These items should run, at least, on the roboRIO. But as a reach goal, bringing back the OpenCV library to run inside of a SmartDashboard plugin was a very attractive option to teams in the past, but we seem to have lost the secret sauce. My understanding is the Ken Streeter and FRC 1591 has resurrected this code and got it working.
• As another reach goal, having Network Tables builds for some of the common co processors would make many peoples lives easier. But not a primary support platform for this team, this is very optional.
• I'd like to see GRIP have the ability to generate a function/class/program or something in one or more languages (C++, Java, Python) that is the few lines of code that implements the pipeline algorithm. In this way, a team could prototype in GRIP, generate code, and stick the code into their robot or dashboard programs.

What do you guys think of these goals?

Brad

At https://github.com/wpilibsuite/allwpilib/blob/master/wpilibj/src/athena/java/edu/wpi/first/wpilibj/PWM.java#L59 it appears that the function is called, but it doesn't actually do anything (it returns a boolean and the value is ignored).

Currently, the HAL returns an invalid handle on any status error, but exceptions are only thrown on negative errors. This means that a HAL handle error will be thrown later, masking an actual issue. We should change the methods that create a HAL handle to always throw on any status error.

They are currently instance methods, but don't use any instance variables, and are explicitly passed in a module. They are designed to be used for global testing, so they should be static.

The method "getImage" in the axisCamera class use a native method called: _Priv_ReadJPEGString_C(image.getAddress(), getByteBufferAddress(string_buf), stringLength);
Aftter several calls to this method it's throws an imaqError -123673104 which is an undocumented exception, this exception is probably because the java GC does not free the image allocated by the API.

This exception has been discussed here: https://www.chiefdelphi.com/forums/showthread.php?t=152212

When CameraServer starts it should remove all entries from the CameraPublisher table.

In https://github.com/wpilibsuite/allwpilib/tree/master/simulation/JavaGazebo/src/main/java/gazebo/msgs,
there is protocol buffers generated code. It appears some of the files are generated from
https://github.com/wpilibsuite/allwpilib/tree/master/simulation/gz_msgs/src/main/proto,
but the rest I couldn't find [in my 10 seconds of searching].

Anyone know where the sources are?

If a team creates a program like this:

public class Robot extends IterativeRobot {
    public void robotInit() {
        Thread t = new Thread(() -> {
            while (true) {
            }
        });
        t.start();
    }
}

The program will fail to die when sent a SIGTERM.

We should document that teams should not do that and instead do this:

public class Robot extends IterativeRobot {
    public void robotInit() {
        Thread t = new Thread(() -> {
            while (!Thread.interrupted()) {
            }
        });
        t.start();
    }
}

or this (but this is worse):

public class Robot extends IterativeRobot {
    public void robotInit() {
        Thread t = new Thread(() -> {
            while (true) {
            }
        });
        t.setDaemon(true);
        t.start();
    }
}

We should probably also ask NI to update frcKillRobot.sh to send a SIGKILL if the user program does not die after a few tenths of seconds.

Currently, including WPILib.h results in including ~344 different headers, including:

• <set> (from Command.h, Scheduler.h, MotorSafetyHelper.h, Ultrasonic.h)

• <queue> (from PIDController.h)

• <list> (from Scheduler.h, CommandGroup.h)

• <map> (from LiveWindow.h, SmartDashboard.h, SendableChooser.h, Scheduler.h, Preferences.h)

• <iostream> (from RobotBase.h)

• <sstream> (from RobotDrive.h, SafePWM.h, MotorSafety.h)

• llvm/DenseMap.h (from CameraServer.h)

This results in ~82400 lines, ~2 MB of included text for the compiler to churn through. While a lot of the included files are due to very common things like <string> and <memory> which can't really be eliminated (and the right solution for users who really care would be to include individual headers rather than WPILib.h), it would be good to re-look at our headers and see if any reductions can be realized for the specific items above through the use of pointer-to-impl (pimpl), changing to different data structures (e.g. std::vector), or other techniques for the non-interface requirements.

While other globals are initialized prior to user class construction, SmartDashboard is not, so if the user class (or any of its instance variables) try to call a SmartDashboard function from within their constructors (rather than e.g. from RobotInit() or later), a crash will result.

The fix should be straightforward: move the SmartDashboard::init() call to either START_ROBOT_CLASS or the RobotBase constructor.

For some features (like gyro calibration) it would be a nice feature to be able to hook into transitions from disabled to autonomous/teleop. Currently this can only be done in the RobotBase class, but a feature to monitor these changes in the DS thread and call a callback would be relatively easy to implement and provide this ability more independently of user code.

Currently, the WPILib HAL was made at an arbitrary point in the code. In most places, it is abstracted at a good spot, however there are some places where major improvements could be made in order to make the HAL cleaner, more of an abstraction layer, and remove some current complications from the code. Below is a list of changes that I feel would improve the abstraction layer both for users, and ease of adding a HAL simulator at a later date.

• Change Joysticks to return -1 to 1: The WPILib level should not be manually doing the conversion from binary values to float values. Changing this would enable cleaner simulation and easier updating the scaler value in future years.
• Change PWM values to take -1 to 1: The WPILib should only care about scaler values, and the HAL should be the one converting that to hardware raw output values. The PWM scaler values would have to be sent to the HAL, but that's a pretty easy change to make.
• Move port resource management from the WPILib to the HAL: There is currently limited resource management in the HAL, but most of it is in the WPILib. The WPILib should not need to do this, and moving this to the HAL would clean up the code and enable easier support in future years. In addition, this would allow us to merge MXP resource and regular resource management, instead of those errors actually throwing different types of errors as they currently do.
• Report port constants and DS data counts using the HAL: It seems like this was attempted to do early, but it was never actually completed. Having the constants for the number of ports in 3 places is hard to maintain.
• Add a Gyro class to the HAL: Currently the WPILib does all the scaling from raw accumulator values to real world values. This is complicated, and seems like the perfect job for a HAL to do. In my opinion, a HAL should always take real world values, and do the internal conversion needed itself.
• Move Encoder and Counter classes to HAL: Currently, counter is OK, but encoders using an encoder when set to 4x, 2 counters when set to 2x, and 1 counter when set to 1x is a complicated abstraction that would be much easier to handle in the HAL. This is currently one of the most complicated class in the WPILib, and one of the most difficult to simulate because you have to check many different things to get the right versions. Plus the raw count scaling changes when different modes are selected.

That’s most of the major changes. There are a few minor ones, mostly involving analog triggers, but I don’t know if it’s possible to do them, and it would be fairly minor anyway.

Anybody have any opinions for or against any of these, or anything else to add?

EDIT: One other thing. Other the PWM and the DS changes, nothing else in the old HAL should need to be changed or removed, so if a team wants use the old HAL methods for some reason they should be able to do so.

I can't find what depends on them. Am I not looking hard enough? Or are the planned to be used in the future?

We have been using the command based paradigm (in C++) in our programs for the past three years, and plan to use it again next year (maybe in Python). We have found that when we create a Command only a few methods are unique. For example, we usually only write code for the constructor and one or two other methods (usually Execute, Initialize, or IsFinished). The rest have empty bodies.

Noticing a pattern, we created some generic classes, which can be found in our GitHub repository. We try to create sensible defaults: Initialize, Execute, and End do nothing; IsFinished returns false; and Interrupted calls End. That has saved us a lot of typing.

We also noticed that certain variations come up a lot. The most noticeable are commands that run for a fixed duration and commands that change one thing and then end. So we created TimedCommand which requires a timeout value and checks IsTimedOut in IsFinished, and InstantCommand which returns true in IsFinished. We then inherit from those to get the behavior we want.

Many of our team members are interested in Python this year, so we are experimenting with it, and the maintainer of robotpy-wpilib asked if we'd be interested in adding helper classes for command based programming to the utilities repository, since currently command based programming is regarded as having too much boilerplate code for Python. When we submitted our pull request, we were told we should mention our changes here, in case there was an interest in changing the default behavior.

The basic idea would be to implement default methods in the Command class (instead of making them pure abstract), and add TimedCommand and InstantCommand alongside the other existing commands. As far as we can see, this would not have any impact on existing robot programs, but it might save teams from typing so much boilerplate when creating new commands.

We can write the C++ code, but someone else would have to handle the Java side.

Note: I have only examined the java implementation, but I assume this holds true for C++ as well.

As a user, if I call disable() on a PIDController, I reasonable expect that after that point PIDWrite will cease to be called. However, a race condition exists that makes this not the case.

In calculate(), the lock is not held while checking enabled -- which may be ok. However, after that line, potentially enabled could be changed, and the calculation is performed. PIDWrite is called after the lock is released, but once again enabled could have changed by that point.

One thing I'm torn on is whether the lock should be held while calling PIDWrite. I feel like it should -- but I also feel that could potentially cause deadlocks if the PIDWrite function called something that locked on something that another thread was holding while calling a PID function. Unsure on the best solution there (maybe use different locks, or perhaps atomic primitives instead?).

As the constructor is private, there is no way to create a BinaryImage without calling a different function (e.g. ColorImage.threshold).

Originally reported at https://www.chiefdelphi.com/forums/showthread.php?threadid=149111

Currently trying to build a SetAllSolenoids function requires iterating through all indexes and calling the CTRE function to set a solenoid. This results in 8 CAN calls, which can be slow. With a SetAllSolenoids function, this would result in one CAN call, and we could build a buffered solenoid setting easier.

For easier vendor use, the HAL should provide a way to do usage reporting with string names instead of enum values. It will need to collate the data from multiple callers and format it (as JSON?) out to a single NetComm usage reporting enum value.

We should be able to stop a VisionRunner in C++. Right now Java teams can do this by calling VisionThread.interupt();.

SendableChooser relies on two ArrayLists to match the name of each option to the object it represents, requiring it to run a check each time you add an option to make sure the option isn't already contained in the SendableChooser. To me it seems using some implementation of the Map interface, such as Hashtable or HashMap, makes more sense.