RSDK-13461: DRAFT! RFC. Fix build and add features#18
Open
stevebriskin wants to merge 8 commits intomainfrom
Open
RSDK-13461: DRAFT! RFC. Fix build and add features#18stevebriskin wants to merge 8 commits intomainfrom
stevebriskin wants to merge 8 commits intomainfrom
Conversation
stevebriskin
changed the title
npmenard
requested changes
Mar 2, 2026
npmenard
left a comment
npmenard
left a comment
There was a problem hiding this comment.
Done with first pass. The new version of the drive driver should allow us to make almost everything async so maybe we can avoid threads?
Agree, delete the CAN module
| with ThreadPoolExecutor(max_workers=1) as executor: | ||
| if odriveSerial.serial_number == "": | ||
| odriveSerial.logger.warning("If you are using multiple Odrive controllers, make sure to add their respective serial_number to each component attributes") | ||
| odriveSerial.odrv = executor.submit(odrive.find_any).result() |
There was a problem hiding this comment.
find_any is an alias for find_sync so you should be able to invoke without a threadexec no?
Author
There was a problem hiding this comment.
Neither worked. something about how the odrive sdk is starting an event loop while one was already started by the viam sdk...i don't recall exactly but i decided that it's not worth debugging for the time being
| self.odrv.axis0.controller.input_vel = rps | ||
|
|
||
| async def reset_zero_position(self, offset: float, *, extra: Optional[Dict[str, Any]] = None, timeout: Optional[float] = None, **kwargs): | ||
| position = await self.get_position() |
There was a problem hiding this comment.
better to use set_abs_pos or set_linear_count
|
|
||
| if odriveSerial.odrv.axis0.config.enable_watchdog: | ||
| odriveSerial.logger.info("Starting watchdog feed thread for axis0, as it is enabled.") | ||
| Thread(target=odriveSerial._periodically_feed_watchdog, daemon=True).start() |
| odriveSerial.vel_limit = odriveSerial.odrv.axis0.controller.config.vel_limit | ||
|
|
||
| odriveSerial._stop_event = Event() | ||
| Thread(target=odriveSerial._periodically_surface_errors, daemon=True).start() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current driver is broken and missing some key features. This is an attempt to fix that. It is best to review this commit by commit....review the first commit from scratch, not as a diff, sorry, it was one big commit.
I focused entirely on the serial model and before merging we need to decide if we should even continue to support the can model if we should just focus on serial and make it good first.
Looking for directional