Relay: add relay output invert function #26568
Conversation
There was a problem hiding this comment.
Can also remove this comment:
ardupilot/libraries/AP_Relay/AP_Relay.cpp
Line 366 in f0fc447
It might also be worth adding a sentence to the DEFAULT parameter description to note that the default is also inverted if invert is set. Or I guess we could make the default ignore inversion.
Isn't it self understanding that invert option inverts the pin output, not the relay state and default option is the relay default state? |
| @@ -65,6 +65,13 @@ const AP_Param::GroupInfo AP_Relay_Params::var_info[] = { | |||
| // @User: Standard | |||
| AP_GROUPINFO("DEFAULT", 3, AP_Relay_Params, default_state, (float)DefaultState::OFF), | |||
|
|
|||
| // @Param: INVERT | |||
| // @DisplayName: Invert output signal | |||
There was a problem hiding this comment.
For the display name it's good to have the subsystem appear first. So perhaps start with "Relay"
We should consider naming this parameter "REVERSED" or "REVERSE" to be consistent with the RC_Channel library with a similar function. It's no big deal but one of the keys to naming stuff is not to introduce new words when we have an existing word that will do.
|
Thanks for the contribution! Could you change the commits to start with "Relay:"? We always try to start commit titles with the subsystem prefixed because it makes it instantly obvious from looking at a commit what part of the system it affects. This makes backporting much easier. .. and just to be clear, I mean the commit title, not the PR title. |
rmackay9
changed the title
i have no idea how to do it. if i try to do it in gitkraken, it makes a new commit in master... EDIT: i hope this is right now |
LaneaLucy
force-pushed
the
relay_output_invert
branch
from
8b0d99a to
1c0913c
Compare
|
btw, someone other could now update the wiki about brushed with relay, because for the L298N Motor Driver there is now no lua script needed anymore. this is also the reason, why i made this PR, because why should i use a lua script for the inversion of the relay outputs, when adding a inversion function to the code only need little change and then you are even free to use every relay output you want without changing lua code... |
tridge
force-pushed
the
relay_output_invert
branch
from
1c0913c to
2cd684f
Compare
|
i've squashed it down to one commit and force pushed |
I have no idea what this means but does this mean everything is ok now? |
tridge
force-pushed
the
relay_output_invert
branch
from
2cd684f to
b060267
Compare
|
@rmackay9 would you prefer "INVERT" or "INVERTED" to "REVERSED" ? |
|
I'd actually prefer we make it REVERSED to be consistent with RC_Channel and SRV_Channel libraries. If we could wind back time, I'd probably make RC_Channel and SRV_Channels use "REVERSE" instread of "REVERSED" but that ship has sailed so let's go with REVERSED for this PR. |
|
Great to hear that AP rover could use the L298N motors without a Lua script. I'd really like to get a wiring diagram and setup instructions into our wiki. I see we have some instructions linked from our wiki here https://ardupilot.org/rover/docs/common-brushed-motors.html but maybe we could replace those with a new page. I could help put that page together I think but I'll need some help from you to confirm it's correct. |
|
@rmackay9 should we discuss this on discord or should I write down it here? |
|
Either place is fine with me. Perhaps discord is a little better because other users may add their comments there as well. |
|
Hi @LaneaLucy, We discussed on the dev call and decided to go with _INVERTED. Sorry for creating the extra work for you. |
|
discussed on the dev call, decided INVERTED is better, I will rework the PR and force push for INVERTED, then mark for merge |
tridge
force-pushed
the
relay_output_invert
branch
from
b060267 to
a6ad4e5
Compare
|
updated and marked for merge |
|
Merged, thanks! |
|
Nice |
added a option for every relay to invert the output signal.
If enabled, Relay on would be Pin output off and Relay off would be Pin output on.