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

Examples aren't using idiomatic C++ #50

Open
calcmogul opened this issue Jan 13, 2021 · 1 comment
Open

Examples aren't using idiomatic C++ #50

calcmogul opened this issue Jan 13, 2021 · 1 comment

Comments

@calcmogul
Copy link

calcmogul commented Jan 13, 2021

One example is https://github.com/CrossTheRoadElec/Phoenix-Examples-Languages/blob/a38b8d193a91eb9deae270daf49bde5730309322/C%2B%2B%20General/MotionProfile/src/main/cpp/Robot.cpp. It would take a while to explain what I mean, so it's easier to just show you. I've included the finished version below with some notes on what I did. WPILib went through a cleanup like this circa 2016, and some of the library features I'm going to mention related to HID were implemented as a result of that process.

  • Replace redudant uses of -1.0 * value or 1.0 * value with -value or value respectively.
  • Pass the constructor arguments where they are used rather than using the constructor initialization list. This is how you'd do it in Java with inline initialization, it's less typing, and there's less syntax to explain.
  • Include what you use. Don't just include WPILib.h. That header will be removed for the 2022 season because users should make their dependencies explicit, and that header massively inflates compilation times.
  • Use XboxController instead of Joystick if you're using a gamepad. The button mappings seem pretty consistent across vendors, so you can use the functions it provides instead of raw button numbers.
  • Use the Get*ButtonPressed()/Get*ButtonReleased() functions in GenericHID (and derived classes) instead of tracking the button states manually.
  • Sort includes lexicographically

I don't have time to look over all the examples, but here's some other things to look out for:

  • Prefer value types over reference types/pointers. That includes value types over smart pointers.
  • Run clang-format on your code with whatever .clang-format config file you like. It'll help keep things consistently formatted.
/**
 * Phoenix Software License Agreement
 *
 * Copyright (C) Cross The Road Electronics.  All rights
 * reserved.
 * 
 * Cross The Road Electronics (CTRE) licenses to you the right to 
 * use, publish, and distribute copies of CRF (Cross The Road) firmware files (*.crf) and 
 * Phoenix Software API Libraries ONLY when in use with CTR Electronics hardware products
 * as well as the FRC roboRIO when in use in FRC Competition.
 * 
 * THE SOFTWARE AND DOCUMENTATION ARE PROVIDED "AS IS" WITHOUT
 * WARRANTY OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT
 * LIMITATION, ANY WARRANTY OF MERCHANTABILITY, FITNESS FOR A
 * PARTICULAR PURPOSE, TITLE AND NON-INFRINGEMENT. IN NO EVENT SHALL
 * CROSS THE ROAD ELECTRONICS BE LIABLE FOR ANY INCIDENTAL, SPECIAL, 
 * INDIRECT OR CONSEQUENTIAL DAMAGES, LOST PROFITS OR LOST DATA, COST OF
 * PROCUREMENT OF SUBSTITUTE GOODS, TECHNOLOGY OR SERVICES, ANY CLAIMS
 * BY THIRD PARTIES (INCLUDING BUT NOT LIMITED TO ANY DEFENSE
 * THEREOF), ANY CLAIMS FOR INDEMNITY OR CONTRIBUTION, OR OTHER
 * SIMILAR COSTS, WHETHER ASSERTED ON THE BASIS OF CONTRACT, TORT
 * (INCLUDING NEGLIGENCE), BREACH OF WARRANTY, OR OTHERWISE
 */
/**
 * This C++ FRC robot application is meant to demonstrate an example using the Motion Profile control mode
 * in Talon SRX.  The CANTalon class gives us the ability to buffer up trajectory points and execute them
 * as the roboRIO streams them into the Talon SRX.
 * 
 * There are many valid ways to use this feature and this example does not sufficiently demonstrate every possible
 * method.  Motion Profile streaming can be as complex as the developer needs it to be for advanced applications,
 * or it can be used in a simple fashion for fire-and-forget actions that require precise timing.
 * 
 * This application is an IterativeRobot project to demonstrate a minimal implementation not requiring the command 
 * framework, however these code excerpts could be moved into a command-based project.
 * 
 * The project also includes Instrumentation.h which simply has debug printfs, and a MotionProfile.h which is generated
 * in @link https://docs.google.com/spreadsheets/d/1PgT10EeQiR92LNXEOEe3VGn737P7WDP4t0CQxQgC8k0/edit#gid=1813770630&vpid=A1
 * or use the Motion Profile Generator.xlsx file in the project folder.
 * 
 * Logitech Gamepad mapping, use left y axis to drive Talon normally.  
 * Press and hold top-left-shoulder-button5 to put Talon into motion profile control mode.
 * This will start sending Motion Profile to Talon while Talon is neutral. 
 * 
 * While holding top-left-shoulder-button5, tap top-right-shoulder-button6.
 * This will signal Talon to fire MP.  When MP is done, Talon will "hold" the last setpoint position
 * and wait for another button6 press to fire again.
 * 
 * Release button5 to allow OpenVoltage control with left y axis.
 */
#include "Constants.h"
#include "Instrumentation.h"
#include "MotionProfileExample.h"
#include "PhysicsSim.h"
#include "ctre/Phoenix.h"
#include "frc/XboxController.h"

class Robot : public frc::TimedRobot {
public:
	/** The Talon we want to motion profile. */
	WPI_TalonSRX _talon{Constants::kTalonID};
	WPI_VictorSPX _vic{Constants::kVictorFollower};

	/** some example logic on how one can manage an MP */
	MotionProfileExample _example{_talon};

	/** gamepad for testing */
	frc::XboxController _controller{0};

	void SimulationInit() {
		PhysicsSim::GetInstance().AddTalonSRX(_talon, 0.75, 2000, true);
		PhysicsSim::GetInstance().AddVictorSPX(_vic);
	}
	void SimulationPeriodic() {
		PhysicsSim::GetInstance().Run();
	}
	void RobotInit(){
	/* Factory Default all hardware to prevent unexpected behaviour */
		_talon.ConfigFactoryDefault();
		_vic.ConfigFactoryDefault();
	}
	/** run once after booting/enter-disable */
	void DisabledInit() {
		_vic.Follow(_talon);
		_talon.ConfigSelectedFeedbackSensor(FeedbackDevice::QuadEncoder, 0,
				kTimeoutMs);
		_talon.SetSensorPhase(true);
		_talon.ConfigNeutralDeadband(Constants::kNeutralDeadbandPercent * 0.01,
				Constants::kTimeoutMs);

		_talon.Config_kF(0, 0.076, kTimeoutMs);
		_talon.Config_kP(0, 2.000, kTimeoutMs);
		_talon.Config_kI(0, 0.0, kTimeoutMs);
		_talon.Config_kD(0, 20.0, kTimeoutMs);

		_talon.ConfigMotionProfileTrajectoryPeriod(10, Constants::kTimeoutMs); //Our profile uses 10 ms timing
		/* status 10 provides the trajectory target for motion profile AND motion magic */
		_talon.SetStatusFramePeriod(StatusFrameEnhanced::Status_10_MotionMagic,
				10, Constants::kTimeoutMs);
	}
	/**  function is called periodically during operator control */
	void TeleopPeriodic() {
		/* call this periodically, and catch the output.  Only apply it if user wants to run MP. */
		_example.control();
		_example.PeriodicTask();

		/* This will signal the robot to start a MP */
		if (_controller.GetBumperPressed(frc::XboxController::kRightHand)) {
			//------------ We could start an MP if MP isn't already running ------------//
			_example.reset();
			_example.start();
		}

		if (_controller.GetBumper(frc::XboxController::kLeftHand)) {
			/* Bumper is held down so switch to motion profile control mode => This is done in MotionProfileControl.
			 * When we transition from no-press to press,
			 * pass a "true" once to MotionProfileControl.
			 */

			_talon.Set(ControlMode::MotionProfile, _example.getSetValue());
		} else {
			/*
			 * If it's not being pressed, just do a simple drive.  This
			 * could be a RobotDrive class or custom drivetrain logic.
			 * The point is we want the switch in and out of MP Control mode.*/

			/* button5 is off so straight drive */
			/* multiple by -1 so joystick forward is positive */
			_talon.Set(ControlMode::PercentOutput,
					       -_controller.GetY(frc::XboxController::kLeftHand));
		}
	}
	/**  function is called periodically during disable */
	void DisabledPeriodic() {
		/* it's generally a good idea to put motor controllers back
		 * into a known state when robot is disabled.  That way when you
		 * enable the robot doesn't just continue doing what it was doing before.
		 * BUT if that's what the application/testing requires than modify this accordingly */
		_talon.Set(ControlMode::PercentOutput, 0);
		/* clear our buffer and put everything into a known state */
		_example.reset();
	}
};

#ifndef RUNNING_FRC_TESTS
int main() { return frc::StartRobot<Robot>(); }
#endif

For what it's worth, this is a useful resource as well, if a bit long (C++ is a big language afterall...): https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines

@JCaporuscio
Copy link
Member

JCaporuscio commented Jan 14, 2021

These look like pretty solid recommendations. We'll likely do one example at a time so that doing them all in a single PR doesn't take forever.

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

No branches or pull requests

2 participants