Skip to content

Fully working B1 ROS navigation on onboard cpu#620

Closed
spomichter wants to merge 13 commits intodevfrom
b1_ros_navigation
Closed

Fully working B1 ROS navigation on onboard cpu#620
spomichter wants to merge 13 commits intodevfrom
b1_ros_navigation

Conversation

@spomichter
Copy link
Contributor

No description provided.

@spomichter spomichter changed the base branch from dev to alexl_ros_bridge September 16, 2025 04:10
Comment on lines +169 to +171
self.stop_timer = threading.Timer(self.command_timeout, self._safety_stop)
self.stop_timer.daemon = True
self.stop_timer.start()
Copy link
Contributor

@paul-nechifor paul-nechifor Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a new thread every time a move is made. I think it would be better to reuse the thread, but that would me creating a custom timer.

Of course, since we're pressed for time, it should be okay for now. Maybe add a TODO: FIXME

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good catch this is bad. Quickly fixing

Base automatically changed from alexl_ros_bridge to dev September 18, 2025 06:19
@spomichter spomichter changed the base branch from dev to main September 18, 2025 11:00
@spomichter spomichter changed the base branch from main to dev September 18, 2025 11:00
@spomichter spomichter changed the base branch from dev to main September 18, 2025 11:22
@spomichter spomichter changed the base branch from main to dev September 18, 2025 11:22
def stop(self):
"""Stop the connection and send stop commands."""
# Cancel timer since we're explicitly stopping
if self.stop_timer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.stop_timer was removed. This will error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes


def cleanup(self):
"""Clean up resources when module is destroyed."""
if self.stop_timer:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

self.stop_timer.cancel()

# Auto-stop after 0.5 seconds if no new commands
self.stop_timer = threading.Timer(self.cmd_vel_timeout, self.stop)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still a timer here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird that this is here in this branch its not in the rebase connection.py

logger.debug(f"Converted to B1Command: {self._current_cmd}")

self.last_command_time = time.time()
self.timeout_active = False # Reset timeout state since we got a new command
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable is modified both here and in the watchdog loop. You need with lock.

Comment on lines +170 to +175
if has_movement and self.current_mode not in (1, 2):
logger.info("Auto-switching to WALK mode for ROS control")
self.set_mode(2)
elif not has_movement and self.current_mode == 2:
logger.info("Auto-switching to IDLE mode (zero velocities)")
self.set_mode(0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NITPICK: These magic numbers should be constants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding enum

Comment on lines +288 to +291
self._current_cmd.lx = 0.0
self._current_cmd.ly = 0.0
self._current_cmd.rx = 0.0
self._current_cmd.ry = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._current_cmd is now used in at least three threads. It needs a lock everywhere it's used.

@spomichter
Copy link
Contributor Author

Resolved by #626

@spomichter spomichter closed this Sep 18, 2025
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

Successfully merging this pull request may close these issues.

3 participants