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

System monitor node #193

Open
wants to merge 9 commits into
base: test/29.04
Choose a base branch
from
Open

Conversation

chrstrom
Copy link
Member

@theoam02 pls allow draft pull requests.

This serves as an example of how a system monitor/supervisor can look. Note the comment in the truster allocator node source. Usually we would let lifecycle management be handled externally, but in this case the only feature we really need is the transition from activated to deactivated. As such, the intialization phase can happen instantaneously on node creation.

@chrstrom chrstrom mentioned this pull request Jul 16, 2024
7 tasks
@chrstrom chrstrom changed the base branch from development to test/29.04 July 16, 2024 17:37
@alekskl01
Copy link
Contributor

alekskl01 commented Jul 17, 2024

I think it covers the functionality we need for now at least. Is there a reason why thruster_forces are set to zero in the system monitor and not as a part of the deactivate transition in thruster_allocator @chrstrom?

@chrstrom
Copy link
Member Author

I think it covers the functionality we need for now at least. Is there a reason why thruster_forces are set to zero in the system monitor and not as a part of the deactivate transition in thruster_allocator @chrstrom?

lifecycle transitions in the allocator are really boilerplate/proof-of-concept at this point, but having both of them publish zero thrust could be a nice piece of redundancy to have?

@chrstrom
Copy link
Member Author

chrstrom commented Aug 8, 2024

@theoam02 @alekskl01 @Andeshog Is this something you want cleaned up and merged into the test branch within the next few days?

@alekskl01
Copy link
Contributor

Yes, that would be great

@chrstrom chrstrom changed the title [DRAFT] System monitor node System monitor node Aug 8, 2024
@chrstrom chrstrom requested review from alekskl01 and Andeshog and removed request for alekskl01 August 8, 2024 14:37
@chrstrom
Copy link
Member Author

chrstrom commented Aug 8, 2024

Ready for review, note that I ended up not having the allocator publish zero-thrust on_shutdown since I ran into some weird deadlocks when disabling the wrench subscriber while an external node was publishing to it. If anyone knows a fix feel free to add it.

Also: Why can there only be 1 reviewer? Would be nice for both @alekskl01 and @Andeshog to look at it and test it on hardware.

Signed-off-by: Christopher Strøm <[email protected]>
@alekskl01
Copy link
Contributor

Hmm, might be a free org private repo thing

@Andeshog
Copy link
Contributor

Andeshog commented Aug 8, 2024

Software killswitch will shut down all the wrench publishers, could that fix the deadlock?

@chrstrom
Copy link
Member Author

chrstrom commented Aug 8, 2024

Software killswitch will shut down all the wrench publishers, could that fix the deadlock?

Maybe, I'm not sure how the node topology looks with that in the mix. I suggest leaving the code as-is until its tested on hardware. An advantage of how things are right now is that the system-monitor node takes full authority without requiring any manual triggers, which could be extended to control other components as well (in the case of low battery voltage f.ex)

Copy link
Contributor

@alekskl01 alekskl01 left a comment

Choose a reason for hiding this comment

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

Not in Trondheim, so i cant test anything on hardwaare

def publish_zero_force(self):
self.get_logger().info('Publishing zero-force on shutdown...')
message = Float32MultiArray()
message.data = [0.0] * 4
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add num_thrusters as a parameter

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