[PATCH] media: rc: meson-ir: add timeout on idle

[PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Meson doesn't seem to be able to generate timeout events
in hardware. So install a software timer to generate the
timeout events required by the decoders to prevent
"ghost keypresses".

Signed-off-by: Matthias Reichl <hias@horus.com>
---
 drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index f2204eb77e2a..f34c5836412b 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -69,6 +69,7 @@ struct meson_ir {
 	void __iomem	*reg;
 	struct rc_dev	*rc;
 	spinlock_t	lock;
+	struct timer_list timeout_timer;
 };
 
 static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
@@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
 	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
 
 	ir_raw_event_store(ir->rc, &rawir);
+
+	mod_timer(&ir->timeout_timer,
+		jiffies + nsecs_to_jiffies(ir->rc->timeout));
+
 	ir_raw_event_handle(ir->rc);
 
 	spin_unlock(&ir->lock);
@@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void meson_ir_timeout_timer(struct timer_list *t)
+{
+	struct meson_ir *ir = from_timer(ir, t, timeout_timer);
+	DEFINE_IR_RAW_EVENT(rawir);
+
+	rawir.timeout = true;
+	rawir.duration = ir->rc->timeout;
+	ir_raw_event_store(ir->rc, &rawir);
+	ir_raw_event_handle(ir->rc);
+}
+
 static int meson_ir_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
 	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
 	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
 	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
+	ir->rc->min_timeout = 1;
 	ir->rc->timeout = MS_TO_NS(200);
+	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	ir->rc->driver_name = DRIVER_NAME;
 
 	spin_lock_init(&ir->lock);
@@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0);
+
 	ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
 	if (ret) {
 		dev_err(dev, "failed to request irq\n");
@@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
 	meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
 	spin_unlock_irqrestore(&ir->lock, flags);
 
+	del_timer_sync(&ir->timeout_timer);
+
 	return 0;
 }
 
-- 
2.11.0

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Sean Young]

Hi Matthias,

On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> Meson doesn't seem to be able to generate timeout events
> in hardware. So install a software timer to generate the
> timeout events required by the decoders to prevent
> "ghost keypresses".
> 
> Signed-off-by: Matthias Reichl <hias@horus.com>
> ---
>  drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index f2204eb77e2a..f34c5836412b 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -69,6 +69,7 @@ struct meson_ir {
>  	void __iomem	*reg;
>  	struct rc_dev	*rc;
>  	spinlock_t	lock;
> +	struct timer_list timeout_timer;
>  };
>  
>  static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
> @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
>  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
>  
>  	ir_raw_event_store(ir->rc, &rawir);
> +
> +	mod_timer(&ir->timeout_timer,
> +		jiffies + nsecs_to_jiffies(ir->rc->timeout));
> +
>  	ir_raw_event_handle(ir->rc);
>  
>  	spin_unlock(&ir->lock);
> @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static void meson_ir_timeout_timer(struct timer_list *t)
> +{
> +	struct meson_ir *ir = from_timer(ir, t, timeout_timer);
> +	DEFINE_IR_RAW_EVENT(rawir);
> +
> +	rawir.timeout = true;
> +	rawir.duration = ir->rc->timeout;
> +	ir_raw_event_store(ir->rc, &rawir);
> +	ir_raw_event_handle(ir->rc);
> +}

Now there can be concurrent access to the raw IR kfifo from the interrupt
handler and the timer. As there is a race condition between the timeout
timer and new IR arriving from the interrupt handler, the timeout could
end being generated after new IR and corrupting a message. There is very
similar functionality in rc-ir-raw.c (with a spinlock).

> +
>  static int meson_ir_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
>  	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>  	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> +	ir->rc->min_timeout = 1;
>  	ir->rc->timeout = MS_TO_NS(200);
> +	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;

Any idea why the default timeout is to 200ms? It seems very high.

>  	ir->rc->driver_name = DRIVER_NAME;
>  
>  	spin_lock_init(&ir->lock);
> @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0);
> +
>  	ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
>  	if (ret) {
>  		dev_err(dev, "failed to request irq\n");
> @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
>  	meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
>  	spin_unlock_irqrestore(&ir->lock, flags);
>  
> +	del_timer_sync(&ir->timeout_timer);
> +
>  	return 0;
>  }
>  
> -- 
> 2.11.0

Would you mind trying this patch?

Thanks

Sean
---
>From f98f4fc05d743ac48a95694996985b2c1f0c4a4b Mon Sep 17 00:00:00 2001
From: Sean Young <sean@mess.org>
Date: Thu, 8 Mar 2018 14:42:44 +0000
Subject: [PATCH] media: rc: meson-ir: add timeout on idle

Meson doesn't seem to be able to generate timeout events in hardware. So
install a software timer to generate the timeout events required by the
decoders to prevent "ghost keypresses".

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/meson-ir.c  |  3 +--
 drivers/media/rc/rc-ir-raw.c | 30 +++++++++++++++++++++++++++---
 include/media/rc-core.h      |  4 +++-
 3 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index f2204eb77e2a..64b0aa4f4db7 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -97,8 +97,7 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
 	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
 	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
 
-	ir_raw_event_store(ir->rc, &rawir);
-	ir_raw_event_handle(ir->rc);
+	ir_raw_event_store_with_timeout(ir->rc, &rawir);
 
 	spin_unlock(&ir->lock);
 
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 984bb82851f9..374f83105a23 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -92,7 +92,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
 {
 	ktime_t			now;
 	DEFINE_IR_RAW_EVENT(ev);
-	int			rc = 0;
 
 	if (!dev->raw)
 		return -EINVAL;
@@ -101,8 +100,33 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
 	ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
 	ev.pulse = !pulse;
 
+	return ir_raw_event_store_with_timeout(dev, &ev);
+}
+EXPORT_SYMBOL_GPL(ir_raw_event_store_edge);
+
+/*
+ * ir_raw_event_store_with_timeout() - pass a pulse/space duration to the raw
+ *				       ir decoders, schedule decoding and
+ *				       timeout
+ * @dev:	the struct rc_dev device descriptor
+ * @ev:		the struct ir_raw_event descriptor of the pulse/space
+ *
+ * This routine (which may be called from an interrupt context) stores a
+ * pulse/space duration for the raw ir decoding state machines, schedules
+ * decoding and generates a timeout.
+ */
+int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev)
+{
+	ktime_t		now;
+	int		rc = 0;
+
+	if (!dev->raw)
+		return -EINVAL;
+
+	now = ktime_get();
+
 	spin_lock(&dev->raw->edge_spinlock);
-	rc = ir_raw_event_store(dev, &ev);
+	rc = ir_raw_event_store(dev, ev);
 
 	dev->raw->last_event = now;
 
@@ -117,7 +141,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
 
 	return rc;
 }
-EXPORT_SYMBOL_GPL(ir_raw_event_store_edge);
+EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout);
 
 /**
  * ir_raw_event_store_with_filter() - pass next pulse/space to decoders with some processing
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index fc3a92668bab..6742fd86ff65 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -334,7 +334,9 @@ void ir_raw_event_handle(struct rc_dev *dev);
 int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev);
 int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse);
 int ir_raw_event_store_with_filter(struct rc_dev *dev,
-				struct ir_raw_event *ev);
+				   struct ir_raw_event *ev);
+int ir_raw_event_store_with_timeout(struct rc_dev *dev,
+				    struct ir_raw_event *ev);
 void ir_raw_event_set_idle(struct rc_dev *dev, bool idle);
 int ir_raw_encode_scancode(enum rc_proto protocol, u32 scancode,
 			   struct ir_raw_event *events, unsigned int max);
-- 
2.14.3

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote:
> On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> > Meson doesn't seem to be able to generate timeout events
> > in hardware. So install a software timer to generate the
> > timeout events required by the decoders to prevent
> > "ghost keypresses".
> > 
> > Signed-off-by: Matthias Reichl <hias@horus.com>
> > ---
> >  drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > index f2204eb77e2a..f34c5836412b 100644
> > --- a/drivers/media/rc/meson-ir.c
> > +++ b/drivers/media/rc/meson-ir.c
> > @@ -69,6 +69,7 @@ struct meson_ir {
> >  	void __iomem	*reg;
> >  	struct rc_dev	*rc;
> >  	spinlock_t	lock;
> > +	struct timer_list timeout_timer;
> >  };
> >  
> >  static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
> > @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> >  
> >  	ir_raw_event_store(ir->rc, &rawir);
> > +
> > +	mod_timer(&ir->timeout_timer,
> > +		jiffies + nsecs_to_jiffies(ir->rc->timeout));
> > +
> >  	ir_raw_event_handle(ir->rc);
> >  
> >  	spin_unlock(&ir->lock);
> > @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static void meson_ir_timeout_timer(struct timer_list *t)
> > +{
> > +	struct meson_ir *ir = from_timer(ir, t, timeout_timer);
> > +	DEFINE_IR_RAW_EVENT(rawir);
> > +
> > +	rawir.timeout = true;
> > +	rawir.duration = ir->rc->timeout;
> > +	ir_raw_event_store(ir->rc, &rawir);
> > +	ir_raw_event_handle(ir->rc);
> > +}
> 
> Now there can be concurrent access to the raw IR kfifo from the interrupt
> handler and the timer. As there is a race condition between the timeout
> timer and new IR arriving from the interrupt handler, the timeout could
> end being generated after new IR and corrupting a message. There is very
> similar functionality in rc-ir-raw.c (with a spinlock).

Ah, thanks for the hint! Now I also noticed your commit a few
weeks ago - must have missed that before.

> > +
> >  static int meson_ir_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
> >  	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
> >  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> >  	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> > +	ir->rc->min_timeout = 1;
> >  	ir->rc->timeout = MS_TO_NS(200);
> > +	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> 
> Any idea why the default timeout is to 200ms? It seems very high.

Indeed it is very high, but I have no idea where that might be
coming from - so I didn't touch it.

I've been testing rc-5 and NEC remotes with 20-50ms timeouts
on meson-ir/upstream kernel and a couple of LibreELEC users are
using 30-50ms timeouts without issues on Amlogic devices as well
(on 3.14 vendor kernel with meson-ir timeout patch):

https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124

Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value
come from? For raw IR signals processed by the decoders this seems
rather high to me as well. On my RPi3 with gpio-ir-recv I'm
using 30ms timeout (with an rc-5 remote) without issues.

> >  	ir->rc->driver_name = DRIVER_NAME;
> >  
> >  	spin_lock_init(&ir->lock);
> > @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > +	timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0);
> > +
> >  	ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
> >  	if (ret) {
> >  		dev_err(dev, "failed to request irq\n");
> > @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
> >  	meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
> >  	spin_unlock_irqrestore(&ir->lock, flags);
> >  
> > +	del_timer_sync(&ir->timeout_timer);
> > +
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.11.0
> 
> Would you mind trying this patch?

Tested-by: Matthias Reichl <hias@horus.com>

Thanks a lot, this patch works fine! And having a common function
in rc-core looks like a very good idea to me as well.

Only thing I'd like to have added is min/max timeout values set
in meson-ir so it's configurable via ir-ctl. A separate patch for
that would make sense, though, I guess.

so long & thanks,

Hias

> 
> Thanks
> 
> Sean
> ---
> >>From f98f4fc05d743ac48a95694996985b2c1f0c4a4b Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Thu, 8 Mar 2018 14:42:44 +0000
> Subject: [PATCH] media: rc: meson-ir: add timeout on idle
> 
> Meson doesn't seem to be able to generate timeout events in hardware. So
> install a software timer to generate the timeout events required by the
> decoders to prevent "ghost keypresses".
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/meson-ir.c  |  3 +--
>  drivers/media/rc/rc-ir-raw.c | 30 +++++++++++++++++++++++++++---
>  include/media/rc-core.h      |  4 +++-
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index f2204eb77e2a..64b0aa4f4db7 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -97,8 +97,7 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
>  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
>  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
>  
> -	ir_raw_event_store(ir->rc, &rawir);
> -	ir_raw_event_handle(ir->rc);
> +	ir_raw_event_store_with_timeout(ir->rc, &rawir);
>  
>  	spin_unlock(&ir->lock);
>  
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 984bb82851f9..374f83105a23 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -92,7 +92,6 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
>  {
>  	ktime_t			now;
>  	DEFINE_IR_RAW_EVENT(ev);
> -	int			rc = 0;
>  
>  	if (!dev->raw)
>  		return -EINVAL;
> @@ -101,8 +100,33 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
>  	ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
>  	ev.pulse = !pulse;
>  
> +	return ir_raw_event_store_with_timeout(dev, &ev);
> +}
> +EXPORT_SYMBOL_GPL(ir_raw_event_store_edge);
> +
> +/*
> + * ir_raw_event_store_with_timeout() - pass a pulse/space duration to the raw
> + *				       ir decoders, schedule decoding and
> + *				       timeout
> + * @dev:	the struct rc_dev device descriptor
> + * @ev:		the struct ir_raw_event descriptor of the pulse/space
> + *
> + * This routine (which may be called from an interrupt context) stores a
> + * pulse/space duration for the raw ir decoding state machines, schedules
> + * decoding and generates a timeout.
> + */
> +int ir_raw_event_store_with_timeout(struct rc_dev *dev, struct ir_raw_event *ev)
> +{
> +	ktime_t		now;
> +	int		rc = 0;
> +
> +	if (!dev->raw)
> +		return -EINVAL;
> +
> +	now = ktime_get();
> +
>  	spin_lock(&dev->raw->edge_spinlock);
> -	rc = ir_raw_event_store(dev, &ev);
> +	rc = ir_raw_event_store(dev, ev);
>  
>  	dev->raw->last_event = now;
>  
> @@ -117,7 +141,7 @@ int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(ir_raw_event_store_edge);
> +EXPORT_SYMBOL_GPL(ir_raw_event_store_with_timeout);
>  
>  /**
>   * ir_raw_event_store_with_filter() - pass next pulse/space to decoders with some processing
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index fc3a92668bab..6742fd86ff65 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -334,7 +334,9 @@ void ir_raw_event_handle(struct rc_dev *dev);
>  int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev);
>  int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse);
>  int ir_raw_event_store_with_filter(struct rc_dev *dev,
> -				struct ir_raw_event *ev);
> +				   struct ir_raw_event *ev);
> +int ir_raw_event_store_with_timeout(struct rc_dev *dev,
> +				    struct ir_raw_event *ev);
>  void ir_raw_event_set_idle(struct rc_dev *dev, bool idle);
>  int ir_raw_encode_scancode(enum rc_proto protocol, u32 scancode,
>  			   struct ir_raw_event *events, unsigned int max);
> -- 
> 2.14.3
> 
> 

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Sean Young]

Hi Matthias,

On Fri, Mar 09, 2018 at 04:54:51PM +0100, Matthias Reichl wrote:
> On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote:
> > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> > > Meson doesn't seem to be able to generate timeout events
> > > in hardware. So install a software timer to generate the
> > > timeout events required by the decoders to prevent
> > > "ghost keypresses".
> > > 
> > > Signed-off-by: Matthias Reichl <hias@horus.com>
> > > ---
> > >  drivers/media/rc/meson-ir.c | 22 ++++++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > > index f2204eb77e2a..f34c5836412b 100644
> > > --- a/drivers/media/rc/meson-ir.c
> > > +++ b/drivers/media/rc/meson-ir.c
> > > @@ -69,6 +69,7 @@ struct meson_ir {
> > >  	void __iomem	*reg;
> > >  	struct rc_dev	*rc;
> > >  	spinlock_t	lock;
> > > +	struct timer_list timeout_timer;
> > >  };
> > >  
> > >  static void meson_ir_set_mask(struct meson_ir *ir, unsigned int reg,
> > > @@ -98,6 +99,10 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> > >  
> > >  	ir_raw_event_store(ir->rc, &rawir);
> > > +
> > > +	mod_timer(&ir->timeout_timer,
> > > +		jiffies + nsecs_to_jiffies(ir->rc->timeout));
> > > +
> > >  	ir_raw_event_handle(ir->rc);
> > >  
> > >  	spin_unlock(&ir->lock);
> > > @@ -105,6 +110,17 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > >  	return IRQ_HANDLED;
> > >  }
> > >  
> > > +static void meson_ir_timeout_timer(struct timer_list *t)
> > > +{
> > > +	struct meson_ir *ir = from_timer(ir, t, timeout_timer);
> > > +	DEFINE_IR_RAW_EVENT(rawir);
> > > +
> > > +	rawir.timeout = true;
> > > +	rawir.duration = ir->rc->timeout;
> > > +	ir_raw_event_store(ir->rc, &rawir);
> > > +	ir_raw_event_handle(ir->rc);
> > > +}
> > 
> > Now there can be concurrent access to the raw IR kfifo from the interrupt
> > handler and the timer. As there is a race condition between the timeout
> > timer and new IR arriving from the interrupt handler, the timeout could
> > end being generated after new IR and corrupting a message. There is very
> > similar functionality in rc-ir-raw.c (with a spinlock).
> 
> Ah, thanks for the hint! Now I also noticed your commit a few
> weeks ago - must have missed that before.
> 
> > > +
> > >  static int meson_ir_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
> > >  	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
> > >  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > >  	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> > > +	ir->rc->min_timeout = 1;
> > >  	ir->rc->timeout = MS_TO_NS(200);
> > > +	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> > 
> > Any idea why the default timeout is to 200ms? It seems very high.
> 
> Indeed it is very high, but I have no idea where that might be
> coming from - so I didn't touch it.
> 
> I've been testing rc-5 and NEC remotes with 20-50ms timeouts
> on meson-ir/upstream kernel and a couple of LibreELEC users are
> using 30-50ms timeouts without issues on Amlogic devices as well
> (on 3.14 vendor kernel with meson-ir timeout patch):
> 
> https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124
> 
> Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value
> come from? For raw IR signals processed by the decoders this seems
> rather high to me as well. On my RPi3 with gpio-ir-recv I'm
> using 30ms timeout (with an rc-5 remote) without issues.

So if the timeout is below N then you will never get a space of N or larger;
the largest space I know of in an IR message is 40ms in the sanyo protocol:

https://www.sbprojects.net/knowledge/ir/sharp.php

So if timeout is set to less than 40ms, we would have trouble decoding the
sharp protocol.

The space between nec repeats is a little less than 100ms. I'm trying to
remember what would could go wrong if the space between them would be
timeouts instead. Mauro do you remember? I can imagine some IR hardware
(e.g. winbond) queuing up IR after generating a timeout, thus delaying
delivering IR to the kernel and we ending up generating a key up.

The problem with a higher timeout is that the trailing space (=timeout)
is sometimes needed for decoding, and decoding of the last message is
delayed until the timeout is received. That means the keyup message is
delayed until that time, making keys a bit "sticky" and more likely to
generate repeats. I'm pretty sure that is needed for rc-5 and nec.

> > >  	ir->rc->driver_name = DRIVER_NAME;
> > >  
> > >  	spin_lock_init(&ir->lock);
> > > @@ -157,6 +175,8 @@ static int meson_ir_probe(struct platform_device *pdev)
> > >  		return ret;
> > >  	}
> > >  
> > > +	timer_setup(&ir->timeout_timer, meson_ir_timeout_timer, 0);
> > > +
> > >  	ret = devm_request_irq(dev, irq, meson_ir_irq, 0, NULL, ir);
> > >  	if (ret) {
> > >  		dev_err(dev, "failed to request irq\n");
> > > @@ -198,6 +218,8 @@ static int meson_ir_remove(struct platform_device *pdev)
> > >  	meson_ir_set_mask(ir, IR_DEC_REG1, REG1_ENABLE, 0);
> > >  	spin_unlock_irqrestore(&ir->lock, flags);
> > >  
> > > +	del_timer_sync(&ir->timeout_timer);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.11.0
> > 
> > Would you mind trying this patch?
> 
> Tested-by: Matthias Reichl <hias@horus.com>
> 
> Thanks a lot, this patch works fine! And having a common function
> in rc-core looks like a very good idea to me as well.
> 
> Only thing I'd like to have added is min/max timeout values set
> in meson-ir so it's configurable via ir-ctl. A separate patch for
> that would make sense, though, I guess.

Yes, that would be good. That should go into a separate commit.

Thanks for testing!

Sean

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote:
> Hi Matthias,
> 
> On Fri, Mar 09, 2018 at 04:54:51PM +0100, Matthias Reichl wrote:
> > On Thu, Mar 08, 2018 at 04:43:27PM +0000, Sean Young wrote:
> > > On Tue, Mar 06, 2018 at 06:41:22PM +0100, Matthias Reichl wrote:
> > > > +
> > > >  static int meson_ir_probe(struct platform_device *pdev)
> > > >  {
> > > >  	struct device *dev = &pdev->dev;
> > > > @@ -145,7 +161,9 @@ static int meson_ir_probe(struct platform_device *pdev)
> > > >  	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
> > > >  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
> > > >  	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> > > > +	ir->rc->min_timeout = 1;
> > > >  	ir->rc->timeout = MS_TO_NS(200);
> > > > +	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
> > > 
> > > Any idea why the default timeout is to 200ms? It seems very high.
> > 
> > Indeed it is very high, but I have no idea where that might be
> > coming from - so I didn't touch it.
> > 
> > I've been testing rc-5 and NEC remotes with 20-50ms timeouts
> > on meson-ir/upstream kernel and a couple of LibreELEC users are
> > using 30-50ms timeouts without issues on Amlogic devices as well
> > (on 3.14 vendor kernel with meson-ir timeout patch):
> > 
> > https://forum.libreelec.tv/thread/11643-le9-0-remote-configs-ir-keytable-amlogic-devices/?postID=83124#post83124
> > 
> > Out of curiosity: where does the 125ms IR_DEFAULT_TIMEOUT value
> > come from? For raw IR signals processed by the decoders this seems
> > rather high to me as well. On my RPi3 with gpio-ir-recv I'm
> > using 30ms timeout (with an rc-5 remote) without issues.
> 
> So if the timeout is below N then you will never get a space of N or larger;
> the largest space I know of in an IR message is 40ms in the sanyo protocol:
> 
> https://www.sbprojects.net/knowledge/ir/sharp.php
> 
> So if timeout is set to less than 40ms, we would have trouble decoding the
> sharp protocol.
> 
> The space between nec repeats is a little less than 100ms. I'm trying to
> remember what would could go wrong if the space between them would be
> timeouts instead. Mauro do you remember? I can imagine some IR hardware
> (e.g. winbond) queuing up IR after generating a timeout, thus delaying
> delivering IR to the kernel and we ending up generating a key up.
> 
> The problem with a higher timeout is that the trailing space (=timeout)
> is sometimes needed for decoding, and decoding of the last message is
> delayed until the timeout is received. That means the keyup message is
> delayed until that time, making keys a bit "sticky" and more likely to
> generate repeats. I'm pretty sure that is needed for rc-5 and nec.

Another issue with high timeouts is the response to very short button
presses where the remote only transmits a single scancode. It then
takes signal transmission time plus timeout, so roughly a quarter
of a second on meson-ir and ite-cir with 200ms timeout, until the
scancode is decoded and the keydown event is generated.

On longer button presses this is less of an issue as we get the
space signal when the first pulse of the repeated scancode is
received. So the delay between button press and keydown is determined
by the remote scancode repeat interval and with typically ~110ms
on nec/rc-5 a lot lower.

This affects both "quick fingers" using a standard remote and
users of programmable remotes like the Logitech Harmony where
the number of scancodes transmitted on a short press can be
configured. With a single scancode transmission we run into
the long keydown delay, 2 scancodes is fine, and at 3 or 4 we
start running into the key repeat issue.

We received several reports from users that their remote felt
"sluggish" when we switched from the downstream "amremote" driver
(which IIRC decoded the nec protocol in hardware) to meson-ir.

Lowering the timeout to 125ms or even significantly lower
(depending on what the protocol and IR receiver permits)
removes this "sluggishness", users report that their remote
is more "responsive".

so long,

Hias

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Sean Young]

Hi Matthias,

On Sat, Mar 10, 2018 at 06:38:28PM +0100, Matthias Reichl wrote:
> On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote:
> > So if the timeout is below N then you will never get a space of N or larger;
> > the largest space I know of in an IR message is 40ms in the sanyo protocol:
> > 
> > https://www.sbprojects.net/knowledge/ir/sharp.php
> > 
> > So if timeout is set to less than 40ms, we would have trouble decoding the
> > sharp protocol.
> > 
> > The space between nec repeats is a little less than 100ms. I'm trying to
> > remember what would could go wrong if the space between them would be
> > timeouts instead. Mauro do you remember? I can imagine some IR hardware
> > (e.g. winbond) queuing up IR after generating a timeout, thus delaying
> > delivering IR to the kernel and we ending up generating a key up.
> > 
> > The problem with a higher timeout is that the trailing space (=timeout)
> > is sometimes needed for decoding, and decoding of the last message is
> > delayed until the timeout is received. That means the keyup message is
> > delayed until that time, making keys a bit "sticky" and more likely to
> > generate repeats. I'm pretty sure that is needed for rc-5 and nec.
> 
> Another issue with high timeouts is the response to very short button
> presses where the remote only transmits a single scancode. It then
> takes signal transmission time plus timeout, so roughly a quarter
> of a second on meson-ir and ite-cir with 200ms timeout, until the
> scancode is decoded and the keydown event is generated.
> 
> On longer button presses this is less of an issue as we get the
> space signal when the first pulse of the repeated scancode is
> received. So the delay between button press and keydown is determined
> by the remote scancode repeat interval and with typically ~110ms
> on nec/rc-5 a lot lower.
> 
> This affects both "quick fingers" using a standard remote and
> users of programmable remotes like the Logitech Harmony where
> the number of scancodes transmitted on a short press can be
> configured. With a single scancode transmission we run into
> the long keydown delay, 2 scancodes is fine, and at 3 or 4 we
> start running into the key repeat issue.
> 
> We received several reports from users that their remote felt
> "sluggish" when we switched from the downstream "amremote" driver
> (which IIRC decoded the nec protocol in hardware) to meson-ir.
> 
> Lowering the timeout to 125ms or even significantly lower
> (depending on what the protocol and IR receiver permits)
> removes this "sluggishness", users report that their remote
> is more "responsive".

That makes complete sense. I'm actually keen to get this lowered, since
this makes it possible to lower the repeat period per-protocol, see
commit d57ea877af38 which had to be reverted (the ite driver will
need fixing up as well before this can happen).

Lowering to below 125ms does increase the risk of regressions, so I
am weary of that. Do you think there is benefit in doing this?

Thanks

Sean

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote:
> Hi Matthias,
> 
> On Sat, Mar 10, 2018 at 06:38:28PM +0100, Matthias Reichl wrote:
> > On Sat, Mar 10, 2018 at 11:27:45AM +0000, Sean Young wrote:
> > > So if the timeout is below N then you will never get a space of N or larger;
> > > the largest space I know of in an IR message is 40ms in the sanyo protocol:
> > > 
> > > https://www.sbprojects.net/knowledge/ir/sharp.php
> > > 
> > > So if timeout is set to less than 40ms, we would have trouble decoding the
> > > sharp protocol.
> > > 
> > > The space between nec repeats is a little less than 100ms. I'm trying to
> > > remember what would could go wrong if the space between them would be
> > > timeouts instead. Mauro do you remember? I can imagine some IR hardware
> > > (e.g. winbond) queuing up IR after generating a timeout, thus delaying
> > > delivering IR to the kernel and we ending up generating a key up.
> > > 
> > > The problem with a higher timeout is that the trailing space (=timeout)
> > > is sometimes needed for decoding, and decoding of the last message is
> > > delayed until the timeout is received. That means the keyup message is
> > > delayed until that time, making keys a bit "sticky" and more likely to
> > > generate repeats. I'm pretty sure that is needed for rc-5 and nec.
> > 
> > Another issue with high timeouts is the response to very short button
> > presses where the remote only transmits a single scancode. It then
> > takes signal transmission time plus timeout, so roughly a quarter
> > of a second on meson-ir and ite-cir with 200ms timeout, until the
> > scancode is decoded and the keydown event is generated.
> > 
> > On longer button presses this is less of an issue as we get the
> > space signal when the first pulse of the repeated scancode is
> > received. So the delay between button press and keydown is determined
> > by the remote scancode repeat interval and with typically ~110ms
> > on nec/rc-5 a lot lower.
> > 
> > This affects both "quick fingers" using a standard remote and
> > users of programmable remotes like the Logitech Harmony where
> > the number of scancodes transmitted on a short press can be
> > configured. With a single scancode transmission we run into
> > the long keydown delay, 2 scancodes is fine, and at 3 or 4 we
> > start running into the key repeat issue.
> > 
> > We received several reports from users that their remote felt
> > "sluggish" when we switched from the downstream "amremote" driver
> > (which IIRC decoded the nec protocol in hardware) to meson-ir.
> > 
> > Lowering the timeout to 125ms or even significantly lower
> > (depending on what the protocol and IR receiver permits)
> > removes this "sluggishness", users report that their remote
> > is more "responsive".
> 
> That makes complete sense. I'm actually keen to get this lowered, since
> this makes it possible to lower the repeat period per-protocol, see
> commit d57ea877af38 which had to be reverted (the ite driver will
> need fixing up as well before this can happen).

I remember the commit, this issue hit us in LibreELEC testbuilds
as well :-)

> Lowering to below 125ms does increase the risk of regressions, so I
> am weary of that. Do you think there is benefit in doing this?

I'd also say stick to the 125ms default. The default settings
should always be safe ones IMO.

People who want to optimize for the last bit of performance can
easily do that on their own, at their own risk.

Personally I've been using gpio-ir-recv on RPi with the default 125ms
timeout and a Hauppauge rc-5 remote for about 2 years now and I've
always been happy with that.

I have to acknowledge though that the responsiveness of a remote
with short messages, like rc-5, in combination with a low timeout
(I tested down to 10ms) is pretty impressive.

so long,

Hias

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Sean Young]

On Mon, Mar 12, 2018 at 02:20:00PM +0100, Matthias Reichl wrote:
> On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote:
> > That makes complete sense. I'm actually keen to get this lowered, since
> > this makes it possible to lower the repeat period per-protocol, see
> > commit d57ea877af38 which had to be reverted (the ite driver will
> > need fixing up as well before this can happen).
> 
> I remember the commit, this issue hit us in LibreELEC testbuilds
> as well :-)
> 
> > Lowering to below 125ms does increase the risk of regressions, so I
> > am weary of that. Do you think there is benefit in doing this?
> 
> I'd also say stick to the 125ms default. The default settings
> should always be safe ones IMO.

Well, yes. I just wanted to explore the ideal situation before making
up our minds.

> People who want to optimize for the last bit of performance can
> easily do that on their own, at their own risk.
> 
> 
> Personally I've been using gpio-ir-recv on RPi with the default 125ms
> timeout and a Hauppauge rc-5 remote for about 2 years now and I've
> always been happy with that.

Ok. We should try to get this change for meson-ir ready for v4.17. I can
write a patch later.

> I have to acknowledge though that the responsiveness of a remote
> with short messages, like rc-5, in combination with a low timeout
> (I tested down to 10ms) is pretty impressive.

I've been thinking about this problem. What we could do is have a 
per-protocol maximum space length, and repeat period. The timeout
can then be set to a maximum space length (+safety margin), and the
keyup timer can be set to timeout + repeat period (+safety margin).

If memory serves, the lirc daemon always sets the timeout with
LIRC_SET_REC_TIMEOUT, so it would not affect lirc daemon decoding.

Anyway, just an idea. Not something for v4.17.

Thanks,

Sean

[PATCH] media: rc: meson-ir: lower timeout and make configurable

[Sean Young]

A timeout of 200ms is much longer than necessary, and delays the decoding
decoding of a single scancode and the last scancode when a button is being
held. This makes the remote seem sluggish.

If the min_timeout and max_timeout values are set, the timeout is
configurable via the LIRC_SET_REC_TIMEOUT ioctl.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/meson-ir.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index 64b0aa4f4db7..f449b35d25e7 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -144,7 +144,9 @@ static int meson_ir_probe(struct platform_device *pdev)
 	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
 	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
 	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
-	ir->rc->timeout = MS_TO_NS(200);
+	ir->rc->min_timeout = 1;
+	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
+	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
 	ir->rc->driver_name = DRIVER_NAME;
 
 	spin_lock_init(&ir->lock);
-- 
2.14.3

Re: [PATCH] media: rc: meson-ir: lower timeout and make configurable

[Matthias Reichl]

On Mon, Mar 12, 2018 at 09:48:40PM +0000, Sean Young wrote:
> A timeout of 200ms is much longer than necessary, and delays the decoding
> decoding of a single scancode and the last scancode when a button is being
> held. This makes the remote seem sluggish.
> 
> If the min_timeout and max_timeout values are set, the timeout is
> configurable via the LIRC_SET_REC_TIMEOUT ioctl.
> 
> Signed-off-by: Sean Young <sean@mess.org>

Tested-by: Matthias Reichl <hias@horus.com>

Thanks a lot, this is working fine!

so long,

Hias

> ---
>  drivers/media/rc/meson-ir.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index 64b0aa4f4db7..f449b35d25e7 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -144,7 +144,9 @@ static int meson_ir_probe(struct platform_device *pdev)
>  	ir->rc->map_name = map_name ? map_name : RC_MAP_EMPTY;
>  	ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
>  	ir->rc->rx_resolution = US_TO_NS(MESON_TRATE);
> -	ir->rc->timeout = MS_TO_NS(200);
> +	ir->rc->min_timeout = 1;
> +	ir->rc->timeout = IR_DEFAULT_TIMEOUT;
> +	ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
>  	ir->rc->driver_name = DRIVER_NAME;
>  
>  	spin_lock_init(&ir->lock);
> -- 
> 2.14.3
> 

Re: [PATCH] media: rc: meson-ir: add timeout on idle

[Matthias Reichl]

Hi Sean,

On Mon, Mar 12, 2018 at 01:58:11PM +0000, Sean Young wrote:
> On Mon, Mar 12, 2018 at 02:20:00PM +0100, Matthias Reichl wrote:
> > On Sun, Mar 11, 2018 at 12:55:19PM +0000, Sean Young wrote:
> > > That makes complete sense. I'm actually keen to get this lowered, since
> > > this makes it possible to lower the repeat period per-protocol, see
> > > commit d57ea877af38 which had to be reverted (the ite driver will
> > > need fixing up as well before this can happen).
> > 
> > I remember the commit, this issue hit us in LibreELEC testbuilds
> > as well :-)
> > 
> > > Lowering to below 125ms does increase the risk of regressions, so I
> > > am weary of that. Do you think there is benefit in doing this?
> > 
> > I'd also say stick to the 125ms default. The default settings
> > should always be safe ones IMO.
> 
> Well, yes. I just wanted to explore the ideal situation before making
> up our minds.
> 
> > People who want to optimize for the last bit of performance can
> > easily do that on their own, at their own risk.
> > 
> > 
> > Personally I've been using gpio-ir-recv on RPi with the default 125ms
> > timeout and a Hauppauge rc-5 remote for about 2 years now and I've
> > always been happy with that.
> 
> Ok. We should try to get this change for meson-ir ready for v4.17. I can
> write a patch later.

Thanks, it worked fine!

> > I have to acknowledge though that the responsiveness of a remote
> > with short messages, like rc-5, in combination with a low timeout
> > (I tested down to 10ms) is pretty impressive.
> 
> I've been thinking about this problem. What we could do is have a 
> per-protocol maximum space length, and repeat period. The timeout
> can then be set to a maximum space length (+safety margin), and the
> keyup timer can be set to timeout + repeat period (+safety margin).

This sounds like a very good idea. It won't help much with IR
receivers that have no configurable timeout or a large minimum
timeout (ite-cir has 100ms min, probably a hardware limitation?),
but for other receivers this'll be a nice improvement.

> If memory serves, the lirc daemon always sets the timeout with
> LIRC_SET_REC_TIMEOUT, so it would not affect lirc daemon decoding.

Current versions of Lirc (0.9.4 and newer) don't seem to use
LIRC_SET_REC_TIMEOUT but handle timeouts on it's own via a
timeout value in poll().

There's still some generic code in lircd.cpp that supports setting
timeouts via LIRC_SET_REC_TIMEOUT but the default plugin (which
handles /dev/lircX) doesn't implement any of the required 
get/set timeout ioctls.

strace on lircd 0.10.0 also shows that only LIRC_GET_FEATURES is
used.

Older Lirc versions (checked with 0.9.1 source I had here)
seem to be using LIRC_SET_REC_TIMEOUT.

So I think we should be fine here.

Not sure if there are other users of the /dev/lirc interface
that could be affected, I'm only familiar with lirc and the
tools from v4l-utils.

> Anyway, just an idea. Not something for v4.17.

No need to rush things, your idea looks good to me but better
test it thoroughly.

Drop me a line if you have a first implementation, I'd be happy
to help with testing.

so long,

Hias