Repository navigation
Add A301 support - #9241
Add A301 support#9241ThadHouse wants to merge 12 commits into
Conversation
For now this is just the HAL, high level will need to wait until the driverslib PR is done
github-actions
Bot
added
component: hal
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e994e6156e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0ee652353
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int32_t stopStatus = 0; | ||
| HAL_StopCANPacketRepeating(a301->canHandle, a301->activeSetpointApi, | ||
| &stopStatus); |
There was a problem hiding this comment.
When the CAN layer returns an error while stopping the previous control API, stopStatus is discarded and the new API is still started. In that failure scenario, the prior periodic setpoint can remain active alongside the new one, yet this call reports only the status of the new send; propagate the cancellation failure and do not begin the new control mode until the old repeating frame is stopped.
Useful? React with 👍 / 👎.
| int32_t setpointStatus = 0; | ||
| WritePacketRepeating(*a301, kDutyCycleSetpointApi, zeroSetpoint.data(), | ||
| zeroSetpoint.size(), &setpointStatus); |
There was a problem hiding this comment.
If the initial repeating CAN write fails, its error is stored only in setpointStatus, while initialization returns a valid handle with the caller's status still zero. This makes clients believe the controller heartbeat and zero command were established and prevents their normal initialization-error recovery; propagate the send error and release the newly allocated A301/CAN resources.
Useful? React with 👍 / 👎.
| A301Obj::~A301Obj() { | ||
| std::scoped_lock lock{gAllocationsMutex}; | ||
| gAllocations.erase(MakeAllocationKey(busId, deviceId)); |
There was a problem hiding this comment.
When simulation calls SimulatorJNI.resetHandles() (or C++ invokes ResetGlobalHandles()), the A301 object is destroyed without HAL_FreeA301(), and this destructor only releases the allocation record. Because the CAN handle resource is reset before the later-created A301 resource, its periodic-send bookkeeping is already gone and no HAL_CAN_SEND_PERIOD_STOP_REPEATING message is emitted, allowing the simulated CAN backend to retain the previous control setpoint across a reset; explicitly stop these sends as part of reset cleanup with the required resource ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ef0de392
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (controlType == HAL_A301_CONTROL_TYPE_VOLTAGE) { | ||
| value /= 12.0; | ||
| } |
There was a problem hiding this comment.
When the controller supply differs from exactly 12 V, as routinely happens under battery load, voltage mode is converted to a fixed duty-cycle fraction and routed through the same CAN API as duty-cycle mode. Consequently, setVoltage(6.0) sends a frame indistinguishable from setThrottle(0.5), so the claimed internal voltage regulation cannot occur and a 10 V bus would produce roughly 5 V rather than 6 V. Use a distinct protocol command if supported, or calculate the duty cycle from the measured bus voltage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Nope, this actually isn't important. A301 handles this internally.
| A301JNI.setStatusFramePeriod( | ||
| m_handle, Objects.requireNonNull(frame, "frame").m_value, periodMs); | ||
| return this; |
There was a problem hiding this comment.
When the period is invalid, the CAN request times out, or the controller rejects the request, the JNI method returns a nonzero HAL status, but this method discards it and still returns this; all of the signal-specific period setters therefore appear successful even though the period was not changed. Return an A301Error, throw on the returned status, or otherwise retain an observable configuration error.
Useful? React with 👍 / 👎.
|
|
||
| int32_t busId; | ||
| int32_t deviceId; | ||
| HAL_CANHandle canHandle; |
There was a problem hiding this comment.
When a handle sets a frame period and is then closed and reopened without power-cycling the A301, the controller retains the configured period while the new object restores these hard-coded defaults. For example, reopening after setting status 2 to 1000 ms caches 20 ms, so ReadPeriodicPacket() applies its 500 ms minimum timeout and marks an otherwise expected frame stale halfway through its real period. Query the effective periods during initialization or explicitly restore the device defaults.
Useful? React with 👍 / 👎.
| HAL_CANHandle canHandle = HAL_InitializeCAN( | ||
| busId, HAL_CANManufacturer::HAL_CAN_MAN_FIRST, deviceId, | ||
| HAL_CANDeviceType::HAL_CAN_DEV_MOTOR_CONTROLLER, status); |
There was a problem hiding this comment.
When production robot code selects CAN_D1 through CAN_D19 and is run in desktop simulation, this initialization returns HAL_PARAMETER_OUT_OF_RANGE: those enum values map to bus IDs 6–24, while hal/src/main/native/sim/PortsInternal.hpp still sets kNumCanBuses to 5 and the simulated HAL_InitializeCAN() rejects IDs greater than 5. (CAN_D0 only slips through because that check uses > rather than >=.) As a result, an A301 configured for nearly any intended Motioncore bus cannot even be constructed in simulation; expand the simulated CAN bus range or provide an A301 simulation path.
Useful? React with 👍 / 👎.
| A301FirmwareVersion version = A301JNI.getFirmwareVersion(m_handle); | ||
| result = "v" + version.major + "." + version.minor + "." + version.patch; | ||
| m_firmwareString = result; |
There was a problem hiding this comment.
When the first getFirmwareString() call times out or encounters another transient CAN read failure, the JNI getter only reports the status and returns the zero-initialized version object, so this code permanently caches v0.0.0. Even after communication recovers, every later call returns that fabricated version without retrying. Only populate m_firmwareString after a successful read, or make the JNI getter surface the failure to this method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26a39e0ae6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (a301->inverted) { | ||
| value *= -1.0; | ||
| } |
There was a problem hiding this comment.
When inversion is enabled, this blanket negation also changes relative-position targets, but HAL_GetA301PeriodicStatus2() returns relativeEncoderPosition without undoing inversion and HAL_SetA301RelativeEncoderPosition() likewise uses the caller's value unchanged. Consequently, after setting the encoder to 0, setRelativePosition(3) sends a target of -3 and reports the reached position as -3, so the controller moves to the opposite location in the API's own coordinate system. Either exclude position controls from this negation or transform all corresponding position reads and encoder setters consistently.
Useful? React with 👍 / 👎.
github-actions
Bot
added
build
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78e9119019
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auto a301 = gA301Handles->Get(handle); | ||
| if (!a301) { | ||
| *status = HAL_HANDLE_ERROR; | ||
| } |
There was a problem hiding this comment.
When a direct HAL caller reuses a nonzero status variable, a successful handle lookup leaves the previous error untouched. Pure operations such as HAL_GetA301BusId() and HAL_SetA301Inverted() therefore appear to fail despite succeeding, while the status 1 and 3 readers see the stale value after GetStatusPeriod() and return before reading CAN data. Set *status = 0 when GetA301() finds a valid handle, consistent with the HAL contract that status is zero on success.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c5d639ce5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int32_t stopStatus = 0; | ||
| HAL_StopCANPacketRepeating(canHandle, api, &stopStatus); |
There was a problem hiding this comment.
If a previous velocity, current, or position frame remains scheduled and its cancellation fails here, stopStatus is discarded; the zero-duty frame can then start successfully and initialization returns a valid handle while the stale API continues repeating alongside it. Propagate the cancellation failure (or retry it) rather than reporting successful initialization with conflicting control frames active.
Useful? React with 👍 / 👎.
| int32_t lastSendStatus = 0; | ||
| for (int32_t retry = 0; retry <= kMaxReadRetries; ++retry) { |
There was a problem hiding this comment.
When a HAL simulation send callback reports an error on one attempt and then succeeds without explicitly overwriting status, HAL_CAN_SendMessage() preserves the incoming value, so lastSendStatus remains nonzero and every remaining retry skips its response read even though the later sends succeeded. Clear lastSendStatus before each send so transient simulated CAN failures can recover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1def23ec5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DRIVERS_HEADER_GEN = [ | ||
| struct( | ||
| class_name = "A301StatusSignal", | ||
| yml_file = "semiwrap/A301StatusSignal.yml", |
There was a problem hiding this comment.
The RobotPy header generator is configured to read semiwrap/A301StatusSignal.yml, but a repo-wide file search confirms that only A301.yml and A301Error.yml were added under drivers/src/main/python/semiwrap. Consequently, the //drivers:wpilib-drivers-generator action cannot resolve this input, preventing the Python bindings and their tests from being generated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2a7147faf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case A301JNI.GEARBOX_RPM_UNKNOWN -> UNKNOWN; | ||
| case A301JNI.GEARBOX_RPM_215 -> RPM_215; | ||
| case A301JNI.GEARBOX_RPM_500 -> RPM_500; | ||
| default -> throw new IllegalArgumentException("Unknown A301 gearbox RPM ID: " + id); |
There was a problem hiding this comment.
When status frame 0 contains a reserved or newly introduced gearbox code (the HAL passes all four encoded bits through), getGearboxRPM() calls this method and throws instead of returning a status signal. The C++ API already treats every value other than the two known RPM codes as kUnknown; make the Java UNKNOWN value provide the same forward-compatible fallback.
Useful? React with 👍 / 👎.
| String result = m_firmwareString; | ||
| if (result == null) { | ||
| A301FirmwareVersion version = A301JNI.getFirmwareVersion(m_handle); | ||
| result = "v" + version.major + "." + version.minor + "." + version.patch; |
There was a problem hiding this comment.
When the controller reports a nonzero prerelease identifier, this always formats the version as an ordinary release and discards that field, so a debug/prerelease firmware build is indistinguishable from the corresponding production build. The C++ implementation in drivers/src/main/native/cpp/motor/A301.cpp explicitly appends the prerelease identifier and Debug Build; apply the same formatting here.
Useful? React with 👍 / 👎.
zachwaffle4
left a comment
zachwaffle4
left a comment
There was a problem hiding this comment.
In general constants should all be SCREAMING_SNAKE_CASE.
| } | ||
| } | ||
|
|
||
| private enum ControlType { |
There was a problem hiding this comment.
I aso think this should be a public sealed interface such that each option can contains its own setpoint in the correct units, and then m_setpoint should be of type ControlType.
|
also, is there still no way to change the closed-loop coefficients? |
| @@ -0,0 +1,199 @@ | |||
| classes: | |||
There was a problem hiding this comment.
The __repr__ implementations from robotpy-rev (robotpy/robotpy-rev#97) should be copied over here.
| using A301StatusSignal<double>::A301StatusSignal; | ||
|
|
||
| /** @return the most recently received value */ | ||
| double Get() const { return A301StatusSignal<double>::Get(); } |
There was a problem hiding this comment.
I'm not sure what your clanker was thinking, but the way rev currently has these signals implemented is much simpler. Do that instead.
3 participants
For now this is just the HAL, high level will need to wait until the driverslib PR is done