[PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4438 bytes --]


Spell "LED" consistently with uppercase.

We do not want people to use LED subsystem for vibrations; there's
already support for that in input subsystem. Remove notes about
vibrations not to confuse people.

Signed-off-by: Pavel Machek <pavel@ucw.cz>


diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
index 3bd38b4..f412603 100644
--- a/Documentation/leds/ledtrig-transient.txt
+++ b/Documentation/leds/ledtrig-transient.txt
@@ -1,7 +1,7 @@
 LED Transient Trigger
 =====================
 
-The leds timer trigger does not currently have an interface to activate
+The LED timer trigger does not currently have an interface to activate
 a one shot timer. The current support allows for setting two timers, one for
 specifying how long a state to be on, and the second for how long the state
 to be off. The delay_on value specifies the time period an LED should stay
@@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
 goes away without deactivating the timer, the hardware will be left in that
 state permanently.
 
-As a specific example of this use-case, let's look at vibrate feature on
-phones. Vibrate function on phones is implemented using PWM pins on SoC or
-PMIC. There is a need to activate one shot timer to control the vibrate
-feature, to prevent user space crashes leaving the phone in vibrate mode
-permanently causing the battery to drain.
-
 Transient trigger addresses the need for one shot timer activation. The
-transient trigger can be enabled and disabled just like the other leds
+transient trigger can be enabled and disabled just like the other LED
 triggers.
 
-When an led class device driver registers itself, it can specify all leds
+When an LED class device driver registers itself, it can specify all LED
 triggers it supports and a default trigger. During registration, activation
 routine for the default trigger gets called. During registration of an led
 class device, the LED state does not change.
@@ -42,12 +36,12 @@ that are active at the time driver gets suspended, continue to run, without
 being able to actually change the LED state. Once driver is resumed, triggers
 start functioning again.
 
-LED state changes are controlled using brightness which is a common led
+LED state changes are controlled using brightness which is a common LED
 class device property. When brightness is set to 0 from user space via
 echo 0 > brightness, it will result in deactivating the current trigger.
 
 Transient trigger uses standard register and unregister interfaces. During
-trigger registration, for each led class device that specifies this trigger
+trigger registration, for each LED class device that specifies this trigger
 as its default trigger, trigger activation routine will get called. During
 registration, the LED state does not change, unless there is another trigger
 active, in which case LED state changes to LED_OFF.
@@ -56,12 +50,12 @@ During trigger unregistration, LED state gets changed to LED_OFF.
 
 Transient trigger activation routine doesn't change the LED state. It
 creates its properties and does its initialization. Transient trigger
-deactivation routine, will cancel any timer that is active before it cleans
+deactivation routine will cancel any timer that is active before it cleans
 up and removes the properties it created. It will restore the LED state to
 non-transient state. When driver gets suspended, irrespective of the transient
 state, the LED state changes to LED_OFF.
 
-Transient trigger can be enabled and disabled from user space on led class
+Transient trigger can be enabled and disabled from user space on LED class
 devices, that support this trigger as shown below:
 
 echo transient > trigger
@@ -144,7 +138,6 @@ repeat the following step as needed:
 	echo none > trigger
 
 This trigger is intended to be used for for the following example use cases:
- - Control of vibrate (phones, tablets etc.) hardware by user space app.
  - Use of LED by user space app as activity indicator.
  - Use of LED by user space app as a kind of watchdog indicator -- as
        long as the app is alive, it can keep the LED illuminated, if it dies

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Jacek Anaszewski]

Hi Pavel,

Thanks for the patch.

On 08/28/2017 11:50 AM, Pavel Machek wrote:
> 
> Spell "LED" consistently with uppercase.
> 
> We do not want people to use LED subsystem for vibrations; there's
> already support for that in input subsystem. Remove notes about
> vibrations not to confuse people.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> 
> diff --git a/Documentation/leds/ledtrig-transient.txt b/Documentation/leds/ledtrig-transient.txt
> index 3bd38b4..f412603 100644
> --- a/Documentation/leds/ledtrig-transient.txt
> +++ b/Documentation/leds/ledtrig-transient.txt
> @@ -1,7 +1,7 @@
>  LED Transient Trigger
>  =====================
>  
> -The leds timer trigger does not currently have an interface to activate
> +The LED timer trigger does not currently have an interface to activate
>  a one shot timer. The current support allows for setting two timers, one for
>  specifying how long a state to be on, and the second for how long the state
>  to be off. The delay_on value specifies the time period an LED should stay
> @@ -16,17 +16,11 @@ set a timer to hold a state, however when user space application crashes or
>  goes away without deactivating the timer, the hardware will be left in that
>  state permanently.
>  
> -As a specific example of this use-case, let's look at vibrate feature on
> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
> -PMIC. There is a need to activate one shot timer to control the vibrate
> -feature, to prevent user space crashes leaving the phone in vibrate mode
> -permanently causing the battery to drain.

I'm not sure if it is a good idea to remove this description. Users will
still be able to use transient trigger this way. It has been around for
five years already and there are users which employ it in this
particular way [0].

Apart from that it's the only documented kernel API for vibrate devices
AFAICT.

>  Transient trigger addresses the need for one shot timer activation. The
> -transient trigger can be enabled and disabled just like the other leds
> +transient trigger can be enabled and disabled just like the other LED
>  triggers.
>  
> -When an led class device driver registers itself, it can specify all leds
> +When an LED class device driver registers itself, it can specify all LED
>  triggers it supports and a default trigger. During registration, activation
>  routine for the default trigger gets called. During registration of an led
>  class device, the LED state does not change.
> @@ -42,12 +36,12 @@ that are active at the time driver gets suspended, continue to run, without
>  being able to actually change the LED state. Once driver is resumed, triggers
>  start functioning again.
>  
> -LED state changes are controlled using brightness which is a common led
> +LED state changes are controlled using brightness which is a common LED
>  class device property. When brightness is set to 0 from user space via
>  echo 0 > brightness, it will result in deactivating the current trigger.
>  
>  Transient trigger uses standard register and unregister interfaces. During
> -trigger registration, for each led class device that specifies this trigger
> +trigger registration, for each LED class device that specifies this trigger
>  as its default trigger, trigger activation routine will get called. During
>  registration, the LED state does not change, unless there is another trigger
>  active, in which case LED state changes to LED_OFF.
> @@ -56,12 +50,12 @@ During trigger unregistration, LED state gets changed to LED_OFF.
>  
>  Transient trigger activation routine doesn't change the LED state. It
>  creates its properties and does its initialization. Transient trigger
> -deactivation routine, will cancel any timer that is active before it cleans
> +deactivation routine will cancel any timer that is active before it cleans
>  up and removes the properties it created. It will restore the LED state to
>  non-transient state. When driver gets suspended, irrespective of the transient
>  state, the LED state changes to LED_OFF.
>  
> -Transient trigger can be enabled and disabled from user space on led class
> +Transient trigger can be enabled and disabled from user space on LED class
>  devices, that support this trigger as shown below:
>  
>  echo transient > trigger
> @@ -144,7 +138,6 @@ repeat the following step as needed:
>  	echo none > trigger
>  
>  This trigger is intended to be used for for the following example use cases:
> - - Control of vibrate (phones, tablets etc.) hardware by user space app.
>   - Use of LED by user space app as activity indicator.
>   - Use of LED by user space app as a kind of watchdog indicator -- as
>         long as the app is alive, it can keep the LED illuminated, if it dies
> 

[0]
https://android.googlesource.com/platform%2Fhardware%2Flibhardware/+/61701df363310a5cbd95e3e1638baa9526e42c9

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]

Hi!

> > -As a specific example of this use-case, let's look at vibrate feature on
> > -phones. Vibrate function on phones is implemented using PWM pins on SoC or
> > -PMIC. There is a need to activate one shot timer to control the vibrate
> > -feature, to prevent user space crashes leaving the phone in vibrate mode
> > -permanently causing the battery to drain.
> 
> I'm not sure if it is a good idea to remove this description. Users will
> still be able to use transient trigger this way. It has been around for
> five years already and there are users which employ it in this
> particular way [0].

I am. Yes, people were doing that, but no, vibration motor is not a
LED. PWM behaviour is different, for example, motor is likely to stop
at low PWM values. We do not want people to do that.

> Apart from that it's the only documented kernel API for vibrate devices
> AFAICT.

Input subsystem has force-feedback protocol, which is very often just
vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
uses that API.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Jacek Anaszewski]

Hi,

On 08/29/2017 10:38 PM, Pavel Machek wrote:
> Hi!
> 
>>> -As a specific example of this use-case, let's look at vibrate feature on
>>> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
>>> -PMIC. There is a need to activate one shot timer to control the vibrate
>>> -feature, to prevent user space crashes leaving the phone in vibrate mode
>>> -permanently causing the battery to drain.
>>
>> I'm not sure if it is a good idea to remove this description. Users will
>> still be able to use transient trigger this way. It has been around for
>> five years already and there are users which employ it in this
>> particular way [0].
> 
> I am. Yes, people were doing that, but no, vibration motor is not a
> LED. PWM behaviour is different, for example, motor is likely to stop
> at low PWM values. We do not want people to do that.

Could you elaborate on how it can be harmful?

I really don't see any merit in removing this from documentation.

You can convince me by collecting some acks from involved people.
I'd like to especially see Rob's opinion. Adding Rob to this thread.

>> Apart from that it's the only documented kernel API for vibrate devices
>> AFAICT.
> 
> Input subsystem has force-feedback protocol, which is very often just
> vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
> uses that API.

Word "vibration" doesn't appear there, so what this patch does
is remove explicit advertisement of kernel support for vibrate
devices without redirecting people to the replacement.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Jani Nikula]

On Tue, 29 Aug 2017, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > -As a specific example of this use-case, let's look at vibrate feature on
>> > -phones. Vibrate function on phones is implemented using PWM pins on SoC or
>> > -PMIC. There is a need to activate one shot timer to control the vibrate
>> > -feature, to prevent user space crashes leaving the phone in vibrate mode
>> > -permanently causing the battery to drain.
>> 
>> I'm not sure if it is a good idea to remove this description. Users will
>> still be able to use transient trigger this way. It has been around for
>> five years already and there are users which employ it in this
>> particular way [0].
>
> I am. Yes, people were doing that, but no, vibration motor is not a
> LED. PWM behaviour is different, for example, motor is likely to stop
> at low PWM values. We do not want people to do that.
>
>> Apart from that it's the only documented kernel API for vibrate devices
>> AFAICT.
>
> Input subsystem has force-feedback protocol, which is very often just
> vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
> uses that API.

N900 as shipped by Nokia used an ad hoc sysfs interface. Because that
sucked, I advocated using the force feedback API for N950 and
N9. Because what is vibration but force feedback? It's a much more
versatile API for vibration than a simple trigger. You get to upload
effects and have the kernel play them for you, also stopping them in a
timely manner regardless of the userspace dying and whatnot. I didn't
double check now, but IIRC you could also link the input to force
feedback, e.g. for touch vibrations.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2091 bytes --]

Hi!

> >> > -As a specific example of this use-case, let's look at vibrate feature on
> >> > -phones. Vibrate function on phones is implemented using PWM pins on SoC or
> >> > -PMIC. There is a need to activate one shot timer to control the vibrate
> >> > -feature, to prevent user space crashes leaving the phone in vibrate mode
> >> > -permanently causing the battery to drain.
> >> 
> >> I'm not sure if it is a good idea to remove this description. Users will
> >> still be able to use transient trigger this way. It has been around for
> >> five years already and there are users which employ it in this
> >> particular way [0].
> >
> > I am. Yes, people were doing that, but no, vibration motor is not a
> > LED. PWM behaviour is different, for example, motor is likely to stop
> > at low PWM values. We do not want people to do that.
> >
> >> Apart from that it's the only documented kernel API for vibrate devices
> >> AFAICT.
> >
> > Input subsystem has force-feedback protocol, which is very often just
> > vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
> > uses that API.
> 
> N900 as shipped by Nokia used an ad hoc sysfs interface. Because that
> sucked, I advocated using the force feedback API for N950 and
> N9. Because what is vibration but force feedback? It's a much more
> versatile API for vibration than a simple trigger. You get to upload
> effects and have the kernel play them for you, also stopping them in a
> timely manner regardless of the userspace dying and whatnot. I didn't
> double check now, but IIRC you could also link the input to force
> feedback, e.g. for touch vibrations.

Ok, N900 support in mainline actually uses force feedback, as in N9,
N950. It is the right interface.

AFAICT, no mainline ARM board uses LEDs for vibrations. And I hope to
keep it that way :-).

(OpenMoko gta01 did that, IIRC. But that is not mainline, good).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2384 bytes --]

On Wed 2017-08-30 22:44:00, Jacek Anaszewski wrote:
> Hi,
> 
> On 08/29/2017 10:38 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>> -As a specific example of this use-case, let's look at vibrate feature on
> >>> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
> >>> -PMIC. There is a need to activate one shot timer to control the vibrate
> >>> -feature, to prevent user space crashes leaving the phone in vibrate mode
> >>> -permanently causing the battery to drain.
> >>
> >> I'm not sure if it is a good idea to remove this description. Users will
> >> still be able to use transient trigger this way. It has been around for
> >> five years already and there are users which employ it in this
> >> particular way [0].

Actually, I checked, and no ARM mainline board does that.

> > I am. Yes, people were doing that, but no, vibration motor is not a
> > LED. PWM behaviour is different, for example, motor is likely to stop
> > at low PWM values. We do not want people to do that.
> 
> Could you elaborate on how it can be harmful?

Well, you can safely route low current to the LEDs. What will it to do
vibration motor, if you leave it on forever? Can the motor safely be
run forever on high current? Not sure.

> I really don't see any merit in removing this from documentation.

There's right API to use for vibrations, and that's force-feedback
support in input/. Not here.

> You can convince me by collecting some acks from involved people.
> I'd like to especially see Rob's opinion. Adding Rob to this thread.

Rob is device tree maintainer. This has little to do with device tree.

> >> Apart from that it's the only documented kernel API for vibrate devices
> >> AFAICT.
> > 
> > Input subsystem has force-feedback protocol, which is very often just
> > vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
> > uses that API.
> 
> Word "vibration" doesn't appear there, so what this patch does
> is remove explicit advertisement of kernel support for vibrate
> devices without redirecting people to the replacement.

Well... this is LED documentation. If there's other documentation
missing somewhere else... we can fix it :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Jacek Anaszewski]

Hi,

On 08/31/2017 10:09 AM, Pavel Machek wrote:
> On Wed 2017-08-30 22:44:00, Jacek Anaszewski wrote:
>> Hi,
>>
>> On 08/29/2017 10:38 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> -As a specific example of this use-case, let's look at vibrate feature on
>>>>> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
>>>>> -PMIC. There is a need to activate one shot timer to control the vibrate
>>>>> -feature, to prevent user space crashes leaving the phone in vibrate mode
>>>>> -permanently causing the battery to drain.
>>>>
>>>> I'm not sure if it is a good idea to remove this description. Users will
>>>> still be able to use transient trigger this way. It has been around for
>>>> five years already and there are users which employ it in this
>>>> particular way [0].
> 
> Actually, I checked, and no ARM mainline board does that.

There might be user space clients like the Android one [0].

>>> I am. Yes, people were doing that, but no, vibration motor is not a
>>> LED. PWM behaviour is different, for example, motor is likely to stop
>>> at low PWM values. We do not want people to do that.
>>
>> Could you elaborate on how it can be harmful?
> 
> Well, you can safely route low current to the LEDs. What will it to do
> vibration motor, if you leave it on forever? Can the motor safely be
> run forever on high current? Not sure.

It's actually one of the main advantages of the "transient" trigger -
you have to re-activate it after the end of each transition cycle,

If you execute the following sequence of commands, then you're getting
1s blink every 3s as long as the while loop is iterating. If you break
the loop the LED output state will be always brought down after the
duration period, No risk that output will be left in the high state
unless transient state was deliberately set to 0.

# echo 1 > state
# echo 1000 > duration
# while [ 1 ] ;do echo 1 > activate ; sleep 3; done


>> I really don't see any merit in removing this from documentation.
> 
> There's right API to use for vibrations, and that's force-feedback
> support in input/. Not here.

Is is as easy to use as this one? It seems that it requires an application
to call ioctl's.

>> You can convince me by collecting some acks from involved people.
>> I'd like to especially see Rob's opinion. Adding Rob to this thread.
> 
> Rob is device tree maintainer. This has little to do with device tree.

I added him because his is the author of the Android patch [0], that
mentions using transient trigger for the LED device named "vibrator".

>>>> Apart from that it's the only documented kernel API for vibrate devices
>>>> AFAICT.
>>>
>>> Input subsystem has force-feedback protocol, which is very often just
>>> vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
>>> uses that API.
>>
>> Word "vibration" doesn't appear there, so what this patch does
>> is remove explicit advertisement of kernel support for vibrate
>> devices without redirecting people to the replacement.
> 
> Well... this is LED documentation. If there's other documentation
> missing somewhere else... we can fix it :-).

We can fix it, but not necessarily remove the valuable information
from this one :-)

[0]
https://android.googlesource.com/platform%2Fhardware%2Flibhardware/+/61701df363310a5cbd95e3e1638baa9526e42c9

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH] Documentation: small fixes for LEDs, hide notes about vibration

[Rob Herring]

On Thu, Aug 31, 2017 at 3:17 PM, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
> Hi,
>
> On 08/31/2017 10:09 AM, Pavel Machek wrote:
>> On Wed 2017-08-30 22:44:00, Jacek Anaszewski wrote:
>>> Hi,
>>>
>>> On 08/29/2017 10:38 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> -As a specific example of this use-case, let's look at vibrate feature on
>>>>>> -phones. Vibrate function on phones is implemented using PWM pins on SoC or
>>>>>> -PMIC. There is a need to activate one shot timer to control the vibrate
>>>>>> -feature, to prevent user space crashes leaving the phone in vibrate mode
>>>>>> -permanently causing the battery to drain.
>>>>>
>>>>> I'm not sure if it is a good idea to remove this description. Users will
>>>>> still be able to use transient trigger this way. It has been around for
>>>>> five years already and there are users which employ it in this
>>>>> particular way [0].
>>
>> Actually, I checked, and no ARM mainline board does that.
>
> There might be user space clients like the Android one [0].

I don't think there is much to worry about there. Unfortunately,
Android is still pretty detached from mainline. Or to put it another
way, this is the least of the problems. We removed timed-gpio which if
anyone had a fixed Android userspace (and tracked mainline) that would
have broken them.

And you're not going to break me, because in fact I have no h/w with a
vibrator and tested all this with an LED and hacks to the DT.

>>>> I am. Yes, people were doing that, but no, vibration motor is not a
>>>> LED. PWM behaviour is different, for example, motor is likely to stop
>>>> at low PWM values. We do not want people to do that.
>>>
>>> Could you elaborate on how it can be harmful?
>>
>> Well, you can safely route low current to the LEDs. What will it to do
>> vibration motor, if you leave it on forever? Can the motor safely be
>> run forever on high current? Not sure.
>
> It's actually one of the main advantages of the "transient" trigger -
> you have to re-activate it after the end of each transition cycle,
>
> If you execute the following sequence of commands, then you're getting
> 1s blink every 3s as long as the while loop is iterating. If you break
> the loop the LED output state will be always brought down after the
> duration period, No risk that output will be left in the high state
> unless transient state was deliberately set to 0.
>
> # echo 1 > state
> # echo 1000 > duration
> # while [ 1 ] ;do echo 1 > activate ; sleep 3; done
>
>
>>> I really don't see any merit in removing this from documentation.
>>
>> There's right API to use for vibrations, and that's force-feedback
>> support in input/. Not here.
>
> Is is as easy to use as this one? It seems that it requires an application
> to call ioctl's.
>
>>> You can convince me by collecting some acks from involved people.
>>> I'd like to especially see Rob's opinion. Adding Rob to this thread.
>>
>> Rob is device tree maintainer. This has little to do with device tree.

Is that all I'm known for? My path of destruction is much wider.

It does affect DT though as we're changing the binding based on what
subsystem we use. Or more accurately, describing as an LED was a
shortcut using an existing binding and perhaps the wrong choice.

> I added him because his is the author of the Android patch [0], that
> mentions using transient trigger for the LED device named "vibrator".

IIRC, the transient trigger was added in the first place for the
vibrator usecase.

>>>>> Apart from that it's the only documented kernel API for vibrate devices
>>>>> AFAICT.
>>>>
>>>> Input subsystem has force-feedback protocol, which is very often just
>>>> vibrations. Documentation/input/ff.rst . Nokia N900 phone actually
>>>> uses that API.
>>>
>>> Word "vibration" doesn't appear there, so what this patch does
>>> is remove explicit advertisement of kernel support for vibrate
>>> devices without redirecting people to the replacement.
>>
>> Well... this is LED documentation. If there's other documentation
>> missing somewhere else... we can fix it :-).
>
> We can fix it, but not necessarily remove the valuable information
> from this one :-)

Beyond documentation, using a GPIO or PWM (or GPIO as a PWM) works
today for at least simple vibrator control (in particular, anyone that
used the timed-gpio Android driver). The force-feedback protocol
should at least support that h/w before we steer folks away from it.

Rob