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.

Sign up for GitHub

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

Jump to bottom

Add CAN rgb led #14710

Closed
wants to merge 5 commits into from
Closed

Add CAN rgb led #14710

wants to merge 5 commits into from

Conversation

CUAVcaijie
Copy link
Contributor

Add CAN rgb led

@julianoes julianoes requested review from dakejahl and dagar April 20, 2020 06:53
TSC21
TSC21 requested changes Apr 20, 2020
View reviewed changes
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

CI is failing do to import failing the led lib in some targets.

src/drivers/uavcan/sensors/rgb.cpp Outdated
@@ -0,0 +1,126 @@
/****************************************************************************
*
* Copyright (c) 2014, 2015 PX4 Development Team. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2014, 2015 PX4 Development Team. All rights reserved.
* Copyright (c) 2020 PX4 Development Team. All rights reserved.

src/drivers/uavcan/sensors/rgb.cpp
UavcanUavcanRgb::init()
{
/*
* Setup timer and call back function for periodic updates
Copy link
Member

Choose a reason for hiding this comment

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

Can you move the description to the header file? Thanks

src/drivers/uavcan/sensors/rgb.hpp Outdated
@@ -0,0 +1,78 @@
/****************************************************************************
*
* Copyright (c) 2014, 2015 PX4 Development Team. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Copyright (c) 2014, 2015 PX4 Development Team. All rights reserved.
* Copyright (c) 2020 PX4 Development Team. All rights reserved.

src/drivers/uavcan/sensors/rgb.hpp

/**
* @file rgb.hpp
*
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a description with @brief annotation? Thanks

src/drivers/uavcan/sensors/rgb.hpp
/**
* @brief The RGB class
*/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

src/drivers/uavcan/sensors/rgb.hpp Outdated
/*
* Max update rate to avoid exessive bus traffic
*/
static constexpr unsigned MAX_RATE_HZ = 100;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static constexpr unsigned MAX_RATE_HZ = 100;
static constexpr unsigned MAX_RATE_HZ = 100;

src/drivers/uavcan/sensors/rgb.hpp Outdated
LedController _led_controller;

/*
* libuavcan related things
Copy link
Member

Choose a reason for hiding this comment

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

Please replace with a description of each object.

src/drivers/uavcan/sensors/rgb.hpp Outdated

typedef uavcan::MethodBinder<UavcanUavcanRgb *, void (UavcanUavcanRgb::*)(const uavcan::TimerEvent &)>
TimerCbBinder;
LedController _led_controller;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LedController _led_controller;
LedController _led_controller;

JacobCrabill
JacobCrabill reviewed Apr 21, 2020
View reviewed changes
src/drivers/uavcan/sensors/rgb.cpp Outdated
#include <systemlib/err.h>


UavcanUavcanRgb::UavcanUavcanRgb(uavcan::INode &node) :
Copy link
Member

Choose a reason for hiding this comment

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

Can we name this something like UavcanRgbLed instead of having UavcanUavcan in the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

JacobCrabill
JacobCrabill requested changes Apr 21, 2020
View reviewed changes
src/drivers/uavcan/sensors/rgb.cpp Outdated
_r = 0; _g = 0; _b = 255;
break;

case led_control_s::COLOR_AMBER: //make it the same as yellow
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
case led_control_s::COLOR_AMBER: //make it the same as yellow
case led_control_s::COLOR_AMBER: // make it the same as yellow

src/drivers/uavcan/sensors/rgb.hpp Outdated
* libuavcan related things
*/
uavcan::Publisher<uavcan::equipment::indication::LightsCommand> _rgb_pub;
uavcan::TimerEventForwarder<TimerCbBinder> _timer;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
uavcan::TimerEventForwarder<TimerCbBinder> _timer;
uavcan::TimerEventForwarder<TimerCbBinder> _timer;

src/drivers/uavcan/sensors/rgb.cpp Outdated
Comment on lines 65 to 66
void
UavcanUavcanRgb::periodic_update(const uavcan::TimerEvent &)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void
UavcanUavcanRgb::periodic_update(const uavcan::TimerEvent &)
void UavcanUavcanRgb::periodic_update(const uavcan::TimerEvent &)

Copy link
Member

Choose a reason for hiding this comment

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

I think I've seen the general PX4 style moving away from the multi-line function header...

bkueng
bkueng reviewed Apr 21, 2020
View reviewed changes
src/drivers/uavcan/sensors/rgb.hpp Outdated
/*
* Max update rate to avoid exessive bus traffic
*/
static constexpr unsigned MAX_RATE_HZ = 100;
Copy link
Member

Choose a reason for hiding this comment

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

100 Hz still seems excessive. What about 20 or 30?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

20 is also is ok , the effect of the breathing light will not be particularly smooth

@weekly-digest weekly-digest bot mentioned this pull request Apr 26, 2020
Closed
@CUAVcaijie CUAVcaijie requested review from JacobCrabill and TSC21 May 26, 2020 01:15
JacobCrabill
JacobCrabill approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@JacobCrabill JacobCrabill left a comment

Choose a reason for hiding this comment

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

Looks good; the only thing I'm wondering about (this is just an organizational thing) is whether this should be under "actuators" instead of "sensors", since an LED is an actuator (a device acting upon a command) rather than a sensor (providing data to the autopilot). Maybe @dagar has an opinion on this as well?

Besides that, looks good.

@dagar
Copy link
Member

dagar commented May 26, 2020

Maybe @dagar has an opinion on this as well?

Yes, but if we're going to do anything here I'd probably want to start by making it a bit more dynamic (at startup). We're paying for a lot of additional state for every possible use case.

@dagar
Copy link
Member

dagar commented May 26, 2020 •
edited
Loading

There's a build failure that needs to be fixed, I'll take a look.

@CUAVcaijie
Copy link
Contributor Author

Thank you

@dagar dagar self-assigned this May 31, 2020
@AlexandreBorowczyk
Copy link

I've tried this PR my branch based on 1.10.1 running on the V5 Nano and I've notice that the board's led won't light up.
Stoping rgbled_pwm and starting it again will fix the issue but will to crash the UavcanNode.

Also should led be added to the DEPENDS in the CMakeLists?

@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Sep 20, 2020
@CUAVcaijie CUAVcaijie closed this Jan 21, 2021
@CUAVcaijie CUAVcaijie deleted the can_rgbled branch January 21, 2021 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkueng bkueng bkueng left review comments

@JacobCrabill JacobCrabill JacobCrabill approved these changes

@dakejahl dakejahl Awaiting requested review from dakejahl

@dagar dagar Awaiting requested review from dagar

@TSC21 TSC21 Awaiting requested review from TSC21

Assignees

@dagar dagar

Labels
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CUAVcaijie @dagar @AlexandreBorowczyk @bkueng @TSC21 @JacobCrabill