[PATCH v2] leds: ledtrig-morse: send out morse code

[PATCH v2] leds: ledtrig-morse: send out morse code

[Andreas Klinger]

Send out a morse code by using LEDs.

This is useful especially on embedded systems without displays to tell the
user about error conditions and status information.

The trigger will be called "morse"

The string to be send is written into the file morse_string and sent out
with a workqueue. Supported are letters and digits.

With the file dot_unit the minimal time unit can be adjusted in
milliseconds.

Also add documentation for the morse led trigger

Thanks to Greg and Geert for suggesting improvements

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../ABI/testing/sysfs-class-led-trigger-morse      |  16 ++
 drivers/leds/trigger/Kconfig                       |  10 +
 drivers/leds/trigger/Makefile                      |   1 +
 drivers/leds/trigger/ledtrig-morse.c               | 298 +++++++++++++++++++++
 4 files changed, 325 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-led-trigger-morse
 create mode 100644 drivers/leds/trigger/ledtrig-morse.c

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-morse b/Documentation/ABI/testing/sysfs-class-led-trigger-morse
new file mode 100644
index 000000000000..dd858e18aef5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-morse
@@ -0,0 +1,16 @@
+What:		/sys/class/leds/<led>/morse_string
+Date:		Jul 2018
+KernelVersion:	4.19
+Contact:	linux-leds@vger.kernel.org
+Description:
+		A string containing alphanumeric characters written to this
+		file is send out as morse code through a LED
+
+What:		/sys/class/leds/<led>/dot_unit
+Date:		Jul 2018
+KernelVersion:	4.19
+Contact:	linux-leds@vger.kernel.org
+Description:
+		Specifies the smallest amount of time in milliseconds of
+		the morse code sent out.
+		Default is 500 ms
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index a2559b4fdfff..ea706ef2354c 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -142,4 +142,14 @@ config LEDS_TRIGGER_NETDEV
 	  This allows LEDs to be controlled by network device activity.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_MORSE
+	tristate "LED Morse Trigger"
+	depends on LEDS_TRIGGERS
+	help
+	  This allows to send a morse code through LEDs.
+	  It is useful especially in embedded systems when there is only
+	  little interface to tell the user error or status codes. Sending
+	  a morse code can be an alternative here.
+	  If unsure, say Y.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index f3cfe1950538..5735381cc3d3 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
 obj-$(CONFIG_LEDS_TRIGGER_NETDEV)	+= ledtrig-netdev.o
+obj-$(CONFIG_LEDS_TRIGGER_MORSE)	+= ledtrig-morse.o
diff --git a/drivers/leds/trigger/ledtrig-morse.c b/drivers/leds/trigger/ledtrig-morse.c
new file mode 100644
index 000000000000..46f1b9d38310
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-morse.c
@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ledtrig-morse: LED Morse Trigger
+ *
+ * send a string as morse code out through LEDs
+ *
+ * can be used to send error codes or messages
+ *
+ * string to be send is written into morse_string
+ * supported are letters and digits
+ *
+ * Author: Andreas Klinger <ak@it-klinger.de>
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/ctype.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/workqueue.h>
+#include <linux/leds.h>
+
+
+#define MORSE_DOT_UNIT_DEFAULT	500
+#define MORSE_TELEGRAM_SIZE	100
+
+struct morse_data {
+	unsigned int		dot_unit;
+	struct led_classdev	*led_cdev;
+	struct work_struct	work;
+	char			telegram[MORSE_TELEGRAM_SIZE];
+	unsigned int		telegram_size;
+	struct mutex		lock;
+};
+
+struct morse_char {
+	char	c;
+	char	*z;
+};
+
+static struct morse_char morse_table[] = {
+	{'a', ".-"},
+	{'b', "-..."},
+	{'c', "-.-."},
+	{'d', "-.."},
+	{'e', "."},
+	{'f', "..-."},
+	{'g', "--."},
+	{'h', "...."},
+	{'i', ".."},
+	{'j', ".---"},
+	{'k', "-.-"},
+	{'l', ".-.."},
+	{'m', "--"},
+	{'n', "-."},
+	{'o', "---"},
+	{'p', ".--."},
+	{'q', "--.-"},
+	{'r', ".-."},
+	{'s', "..."},
+	{'t', "-"},
+	{'u', "..-"},
+	{'v', "...-"},
+	{'w', ".--"},
+	{'x', "-..-"},
+	{'y', "-.--"},
+	{'z', "--.."},
+	{'1', ".----"},
+	{'2', "..---"},
+	{'3', "...--"},
+	{'4', "....-"},
+	{'5', "....."},
+	{'6', "-...."},
+	{'7', "--..."},
+	{'8', "---.."},
+	{'9', "----."},
+	{'0', "-----"},
+	{0, NULL},
+};
+
+static void morse_long(struct led_classdev *led_cdev)
+{
+	struct morse_data *data = led_cdev->trigger_data;
+
+	led_set_brightness(led_cdev, LED_ON);
+	msleep(3 * data->dot_unit);
+	led_set_brightness(led_cdev, LED_OFF);
+	msleep(data->dot_unit);
+}
+
+static void morse_short(struct led_classdev *led_cdev)
+{
+	struct morse_data *data = led_cdev->trigger_data;
+
+	led_set_brightness(led_cdev, LED_ON);
+	msleep(data->dot_unit);
+	led_set_brightness(led_cdev, LED_OFF);
+	msleep(data->dot_unit);
+}
+
+static void morse_letter_space(struct led_classdev *led_cdev)
+{
+	struct morse_data *data = led_cdev->trigger_data;
+	/*
+	 * Pause: 3 dot spaces
+	 * 1 dot space already there from morse character
+	 */
+	msleep(2 * data->dot_unit);
+}
+
+static void morse_word_space(struct led_classdev *led_cdev)
+{
+	struct morse_data *data = led_cdev->trigger_data;
+	/*
+	 * Pause: 7 dot spaces
+	 * 1 dot space already there from morse character
+	 * 2 dot spaces already there from letter space
+	 */
+	msleep(4 * data->dot_unit);
+}
+
+static void morse_send_char(struct led_classdev *led_cdev, char ch)
+{
+	unsigned int i = 0;
+
+	while ((morse_table[i].c) && (morse_table[i].c != tolower(ch)))
+		i++;
+
+	if (morse_table[i].c) {
+		unsigned int j = 0;
+
+		while (morse_table[i].z[j]) {
+			switch (morse_table[i].z[j]) {
+			case '.':
+				morse_short(led_cdev);
+				break;
+			case '-':
+				morse_long(led_cdev);
+				break;
+			}
+			j++;
+		}
+		morse_letter_space(led_cdev);
+	} else {
+		/*
+		 * keep it simple:
+		 * whenever there is an unrecognized character make a word
+		 * space
+		 */
+		morse_word_space(led_cdev);
+	}
+}
+
+static void morse_work(struct work_struct *work)
+{
+	struct morse_data *data = container_of(work, struct morse_data, work);
+	unsigned int i;
+
+	mutex_lock(&data->lock);
+
+	for (i = 0; i < data->telegram_size; i++)
+		morse_send_char(data->led_cdev, data->telegram[i]);
+
+	mutex_unlock(&data->lock);
+}
+
+static ssize_t morse_string_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct morse_data *data = led_cdev->trigger_data;
+
+	if (size >= sizeof(data->telegram))
+		return -E2BIG;
+
+	mutex_lock(&data->lock);
+
+	memcpy(data->telegram, buf, size);
+	data->telegram_size = size;
+
+	mutex_unlock(&data->lock);
+
+	schedule_work(&data->work);
+
+	return size;
+}
+
+static DEVICE_ATTR_WO(morse_string);
+
+static ssize_t dot_unit_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct morse_data *data = led_cdev->trigger_data;
+
+	return sprintf(buf, "%u\n", data->dot_unit);
+}
+
+static ssize_t dot_unit_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct morse_data *data = led_cdev->trigger_data;
+	unsigned long dot_unit;
+	ssize_t ret = -EINVAL;
+
+	ret = kstrtoul(buf, 10, &dot_unit);
+	if (ret)
+		return ret;
+
+	data->dot_unit = dot_unit;
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(dot_unit);
+
+static void morse_trig_activate(struct led_classdev *led_cdev)
+{
+	int rc;
+	struct morse_data *data;
+
+	data = kzalloc(sizeof(struct morse_data), GFP_KERNEL);
+	if (!data) {
+		dev_err(led_cdev->dev, "unable to allocate morse trigger\n");
+		return;
+	}
+
+	led_cdev->trigger_data = data;
+	data->led_cdev = led_cdev;
+	data->dot_unit = MORSE_DOT_UNIT_DEFAULT;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_morse_string);
+	if (rc)
+		goto err_out_data;
+
+	rc = device_create_file(led_cdev->dev, &dev_attr_dot_unit);
+	if (rc)
+		goto err_out_morse_string;
+
+	INIT_WORK(&data->work, morse_work);
+
+	mutex_init(&data->lock);
+
+	led_set_brightness(led_cdev, LED_OFF);
+	led_cdev->activated = true;
+
+	return;
+
+err_out_data:
+	kfree(data);
+err_out_morse_string:
+	device_remove_file(led_cdev->dev, &dev_attr_morse_string);
+}
+
+static void morse_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct morse_data *data = led_cdev->trigger_data;
+
+	if (led_cdev->activated) {
+
+		cancel_work_sync(&data->work);
+
+		device_remove_file(led_cdev->dev, &dev_attr_morse_string);
+		device_remove_file(led_cdev->dev, &dev_attr_dot_unit);
+
+		kfree(data);
+
+		led_cdev->trigger_data = NULL;
+		led_cdev->activated = false;
+	}
+}
+
+static struct led_trigger morse_led_trigger = {
+	.name     = "morse",
+	.activate = morse_trig_activate,
+	.deactivate = morse_trig_deactivate,
+};
+
+static int __init morse_trig_init(void)
+{
+	return led_trigger_register(&morse_led_trigger);
+}
+
+static void __exit morse_trig_exit(void)
+{
+	led_trigger_unregister(&morse_led_trigger);
+}
+
+module_init(morse_trig_init);
+module_exit(morse_trig_exit);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("Morse code LED trigger");
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Andy Shevchenko]

On Tue, Jul 3, 2018 at 6:53 PM, Andreas Klinger <ak@it-klinger.de> wrote:
> Send out a morse code by using LEDs.
>
> This is useful especially on embedded systems without displays to tell the
> user about error conditions and status information.
>
> The trigger will be called "morse"
>
> The string to be send is written into the file morse_string and sent out
> with a workqueue. Supported are letters and digits.
>
> With the file dot_unit the minimal time unit can be adjusted in
> milliseconds.
>
> Also add documentation for the morse led trigger


> @@ -0,0 +1,298 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ledtrig-morse: LED Morse Trigger
> + *
> + * send a string as morse code out through LEDs
> + *
> + * can be used to send error codes or messages
> + *
> + * string to be send is written into morse_string
> + * supported are letters and digits
> + *
> + * Author: Andreas Klinger <ak@it-klinger.de>

> + *

Redundant line.

> + */

> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/ctype.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/workqueue.h>
> +#include <linux/leds.h>

If you keep it sorted it might be better to avoid any redundancy or
duplication in the future.

For now init.h and module.h are not needed together. Choose one which
suits (I guess module.h).

> +
> +

Redundant one blank line.

> +#define MORSE_DOT_UNIT_DEFAULT 500

If it's time, put it's unit to the end, like _MS

> +#define MORSE_TELEGRAM_SIZE    100

I would rather choose power of two -ish number, like
96 or 128.

> +
> +struct morse_data {
> +       unsigned int            dot_unit;
> +       struct led_classdev     *led_cdev;
> +       struct work_struct      work;
> +       char                    telegram[MORSE_TELEGRAM_SIZE];
> +       unsigned int            telegram_size;
> +       struct mutex            lock;
> +};
> +
> +struct morse_char {
> +       char    c;
> +       char    *z;
> +};
> +

> +static struct morse_char morse_table[] = {

const ?

> +       {'a', ".-"},
> +       {'b', "-..."},
> +       {'c', "-.-."},
> +       {'d', "-.."},
> +       {'e', "."},
> +       {'f', "..-."},
> +       {'g', "--."},
> +       {'h', "...."},
> +       {'i', ".."},
> +       {'j', ".---"},
> +       {'k', "-.-"},
> +       {'l', ".-.."},
> +       {'m', "--"},
> +       {'n', "-."},
> +       {'o', "---"},
> +       {'p', ".--."},
> +       {'q', "--.-"},
> +       {'r', ".-."},
> +       {'s', "..."},
> +       {'t', "-"},
> +       {'u', "..-"},
> +       {'v', "...-"},
> +       {'w', ".--"},
> +       {'x', "-..-"},
> +       {'y', "-.--"},
> +       {'z', "--.."},
> +       {'1', ".----"},
> +       {'2', "..---"},
> +       {'3', "...--"},
> +       {'4', "....-"},
> +       {'5', "....."},
> +       {'6', "-...."},
> +       {'7', "--..."},
> +       {'8', "---.."},
> +       {'9', "----."},
> +       {'0', "-----"},

Do you expect this to be changed somehow?
Otherwise we might just to keep two char arrays of alphas and digits
in an order of ascii appearance.

In the code something like

ch = tolower(x);
if (isalpha(ch))
 code = alphas[ch - 'a'];
else if (isdigit(ch))
 code = digits[ch - '0'];
else
 code = unknown;

> +       {0, NULL},

And this will gone, you just provide it with known size,

> +};

> +       msleep(3 * data->dot_unit);
> +       msleep(data->dot_unit);

For sake of consistency I would rather use

 1 * ...

> +static void morse_send_char(struct led_classdev *led_cdev, char ch)
> +{
> +       unsigned int i = 0;
> +
> +       while ((morse_table[i].c) && (morse_table[i].c != tolower(ch)))
> +               i++;
> +
> +       if (morse_table[i].c) {
> +               unsigned int j = 0;
> +
> +               while (morse_table[i].z[j]) {
> +                       switch (morse_table[i].z[j]) {
> +                       case '.':
> +                               morse_short(led_cdev);
> +                               break;
> +                       case '-':
> +                               morse_long(led_cdev);
> +                               break;
> +                       }
> +                       j++;
> +               }
> +               morse_letter_space(led_cdev);
> +       } else {
> +               /*
> +                * keep it simple:
> +                * whenever there is an unrecognized character make a word
> +                * space
> +                */
> +               morse_word_space(led_cdev);
> +       }
> +}


> +static ssize_t morse_string_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct morse_data *data = led_cdev->trigger_data;
> +
> +       if (size >= sizeof(data->telegram))
> +               return -E2BIG;

Hmm... What's wrong if it equals the size? Do we care about '\0' at
the end here?

> +
> +       mutex_lock(&data->lock);
> +
> +       memcpy(data->telegram, buf, size);
> +       data->telegram_size = size;
> +
> +       mutex_unlock(&data->lock);


> +
> +       schedule_work(&data->work);
> +
> +       return size;
> +}
> +
> +static DEVICE_ATTR_WO(morse_string);
> +
> +static ssize_t dot_unit_show(struct device *dev,
> +                               struct device_attribute *attr, char *buf)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct morse_data *data = led_cdev->trigger_data;
> +
> +       return sprintf(buf, "%u\n", data->dot_unit);
> +}
> +
> +static ssize_t dot_unit_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t size)
> +{
> +       struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +       struct morse_data *data = led_cdev->trigger_data;
> +       unsigned long dot_unit;

> +       ssize_t ret = -EINVAL;

Redundant assignment.

> +
> +       ret = kstrtoul(buf, 10, &dot_unit);
> +       if (ret)
> +               return ret;
> +
> +       data->dot_unit = dot_unit;
> +
> +       return size;
> +}
> +
> +static DEVICE_ATTR_RW(dot_unit);
> +
> +static void morse_trig_activate(struct led_classdev *led_cdev)
> +{
> +       int rc;
> +       struct morse_data *data;
> +
> +       data = kzalloc(sizeof(struct morse_data), GFP_KERNEL);
> +       if (!data) {
> +               dev_err(led_cdev->dev, "unable to allocate morse trigger\n");
> +               return;
> +       }
> +
> +       led_cdev->trigger_data = data;
> +       data->led_cdev = led_cdev;
> +       data->dot_unit = MORSE_DOT_UNIT_DEFAULT;
> +

> +       rc = device_create_file(led_cdev->dev, &dev_attr_morse_string);
> +       if (rc)
> +               goto err_out_data;
> +
> +       rc = device_create_file(led_cdev->dev, &dev_attr_dot_unit);
> +       if (rc)
> +               goto err_out_morse_string;

When file appears, it means it's possible immediately to store/show.
Now imagine what would happen.

> +
> +       INIT_WORK(&data->work, morse_work);
> +
> +       mutex_init(&data->lock);
> +
> +       led_set_brightness(led_cdev, LED_OFF);
> +       led_cdev->activated = true;
> +
> +       return;
> +
> +err_out_data:
> +       kfree(data);
> +err_out_morse_string:
> +       device_remove_file(led_cdev->dev, &dev_attr_morse_string);
> +}
> +

> +static void morse_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +       struct morse_data *data = led_cdev->trigger_data;
> +

> +       if (led_cdev->activated) {

Perhaps negative condition?

> +
> +               cancel_work_sync(&data->work);
> +
> +               device_remove_file(led_cdev->dev, &dev_attr_morse_string);
> +               device_remove_file(led_cdev->dev, &dev_attr_dot_unit);
> +
> +               kfree(data);
> +
> +               led_cdev->trigger_data = NULL;
> +               led_cdev->activated = false;
> +       }
> +}

> +MODULE_LICENSE("GPL");

This is not aligned with SPDX id.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Willy Tarreau]

On Tue, Jul 03, 2018 at 09:43:06PM +0300, Andy Shevchenko wrote:
> > +struct morse_char {
> > +       char    c;
> > +       char    *z;
> > +};
> > +
> 
> > +static struct morse_char morse_table[] = {
> 
> const ?
> 
> > +       {'a', ".-"},
> > +       {'b', "-..."},
> > +       {'c', "-.-."},
> > +       {'d', "-.."},
> > +       {'e', "."},
> > +       {'f', "..-."},
> > +       {'g', "--."},
> > +       {'h', "...."},
> > +       {'i', ".."},
> > +       {'j', ".---"},
> > +       {'k', "-.-"},
> > +       {'l', ".-.."},
> > +       {'m', "--"},
> > +       {'n', "-."},
> > +       {'o', "---"},
> > +       {'p', ".--."},
> > +       {'q', "--.-"},
> > +       {'r', ".-."},
> > +       {'s', "..."},
> > +       {'t', "-"},
> > +       {'u', "..-"},
> > +       {'v', "...-"},
> > +       {'w', ".--"},
> > +       {'x', "-..-"},
> > +       {'y', "-.--"},
> > +       {'z', "--.."},
> > +       {'1', ".----"},
> > +       {'2', "..---"},
> > +       {'3', "...--"},
> > +       {'4', "....-"},
> > +       {'5', "....."},
> > +       {'6', "-...."},
> > +       {'7', "--..."},
> > +       {'8', "---.."},
> > +       {'9', "----."},
> > +       {'0', "-----"},
> 
> Do you expect this to be changed somehow?
> Otherwise we might just to keep two char arrays of alphas and digits
> in an order of ascii appearance.
> 
> In the code something like
> 
> ch = tolower(x);
> if (isalpha(ch))
>  code = alphas[ch - 'a'];
> else if (isdigit(ch))
>  code = digits[ch - '0'];
> else
>  code = unknown;
> 
> > +       {0, NULL},
> 
> And this will gone, you just provide it with known size,

Well, in this case it's even possible to go further and avoid storing
36 strings. Indeed, no representation is longer than 5 symbols, so you
can use 5 bits for the encoding (0=".", 1="-") and 3 bits for the
length, it gives you a single byte per character instead of a pointer
to a string plus 6 chars. Then in order to make it readable, 5 macros
can be provided to emit the code :

#define MORSE1(a,b)       (1 | ((a)<<3))
#define MORSE2(a,b)       (2 | ((a)<<3)|((b)<<4))
#define MORSE3(a,b,c)     (3 | ((a)<<3)|((b)<<4)|((c)<<5))
#define MORSE4(a,b,c,d)   (4 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6))
#define MORSE5(a,b,c,d,e) (5 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6)|((e)<<7))

Then all chars may be defined like this :

    ['a'] = MORSE2(0,1),
    ['b'] = MORSE4(1,0,0,0),
    ['c'] = MORSE4(1,0,1,0),
    ['d'] = MORSE3(1,0,0),
    ['e'] = MORSE1(0),
    ...

and when processing these :

    code = morse_table[tolower(c)];
    code_len = code & 7;
    code >>= 3;

    while (code_len) {
       if (code & 1)
           emit_long();
       else
           emit_short();
       code >>= 1;
       code_len--;
    }

In this case it could even cover the whole ASCII table at once since it's
not certain that the saved bytes compensate for the extra code and alignment
used to save them :-)

Note that I'm not suggesting that it is required to proceed like this, but
I think it makes the whole code more compact, which aligns with the purpose
of focusing on embedded devices.

Cheers,
Willy

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Pavel Machek]

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

On Tue 2018-07-03 17:53:28, Andreas Klinger wrote:
> Send out a morse code by using LEDs.
> 
> This is useful especially on embedded systems without displays to tell the
> user about error conditions and status information.
> 
> The trigger will be called "morse"
> 
> The string to be send is written into the file morse_string and sent out
> with a workqueue. Supported are letters and digits.
> 
> With the file dot_unit the minimal time unit can be adjusted in
> milliseconds.
> 
> Also add documentation for the morse led trigger
> 
> Thanks to Greg and Geert for suggesting improvements
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

As I stated before, I don't think morse encoder belongs in kernel.

LED pattern trigger should be merged, instead.
									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 v2] leds: ledtrig-morse: send out morse code

[Willy Tarreau]

Hi Pavel,

On Wed, Jul 04, 2018 at 08:53:05AM +0200, Pavel Machek wrote:
> As I stated before, I don't think morse encoder belongs in kernel.

On the opposite, I think that the kernel needs to be a bit more autonomous
when it comes to reporting its own issues. Being able to report a panic
when userland cannot be accessed for example is the reason why we've seen
various features such as blinking keyboard LEDs for this.

> LED pattern trigger should be merged, instead.

Well, just like we have LED and LED triggers in the kernel, I think having
a generic way to use patterns could be nice and in this case Morse could be
one such pattern, but if that means it's limited to userland to configure
it then it sadly voids all of its benefits.

Last, as I showed on my previous mail in this thread, the Morse encoding
can be brought to 36 bytes, which is much less than even the registration
code needed to feed it. At some point we need to focus on code efficiency
and doing things right instead of how too many layers look like from far
away. While I don't need Morse right now I consider it as a nice addition
I would definitely enable by default on all my boards if it helps provide
details about what prevents my system from booting or what just killed it.

Cheers,
Willy

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Geert Uytterhoeven]

Hi Willy,

On Wed, Jul 4, 2018 at 4:41 AM Willy Tarreau <w@1wt.eu> wrote:
> On Tue, Jul 03, 2018 at 09:43:06PM +0300, Andy Shevchenko wrote:
> > > +       {'a', ".-"},
> > > +       {'b', "-..."},

> >
> > Do you expect this to be changed somehow?
> > Otherwise we might just to keep two char arrays of alphas and digits
> > in an order of ascii appearance.
> >
> > In the code something like
> >
> > ch = tolower(x);
> > if (isalpha(ch))
> >  code = alphas[ch - 'a'];
> > else if (isdigit(ch))
> >  code = digits[ch - '0'];
> > else
> >  code = unknown;
> >
> > > +       {0, NULL},
> >
> > And this will gone, you just provide it with known size,
>
> Well, in this case it's even possible to go further and avoid storing
> 36 strings. Indeed, no representation is longer than 5 symbols, so you
> can use 5 bits for the encoding (0=".", 1="-") and 3 bits for the
> length, it gives you a single byte per character instead of a pointer
> to a string plus 6 chars. Then in order to make it readable, 5 macros

Hehe, https://lkml.org/lkml/2018/6/28/544 ;-)

> can be provided to emit the code :
>
> #define MORSE1(a,b)       (1 | ((a)<<3))
> #define MORSE2(a,b)       (2 | ((a)<<3)|((b)<<4))
> #define MORSE3(a,b,c)     (3 | ((a)<<3)|((b)<<4)|((c)<<5))
> #define MORSE4(a,b,c,d)   (4 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6))
> #define MORSE5(a,b,c,d,e) (5 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6)|((e)<<7))
>
> Then all chars may be defined like this :
>
>     ['a'] = MORSE2(0,1),
>     ['b'] = MORSE4(1,0,0,0),
>     ['c'] = MORSE4(1,0,1,0),
>     ['d'] = MORSE3(1,0,0),
>     ['e'] = MORSE1(0),

Nice!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Greg KH]

On Wed, Jul 04, 2018 at 09:34:43AM +0200, Willy Tarreau wrote:
> Hi Pavel,
> 
> On Wed, Jul 04, 2018 at 08:53:05AM +0200, Pavel Machek wrote:
> > As I stated before, I don't think morse encoder belongs in kernel.
> 
> On the opposite, I think that the kernel needs to be a bit more autonomous
> when it comes to reporting its own issues. Being able to report a panic
> when userland cannot be accessed for example is the reason why we've seen
> various features such as blinking keyboard LEDs for this.
> 
> > LED pattern trigger should be merged, instead.
> 
> Well, just like we have LED and LED triggers in the kernel, I think having
> a generic way to use patterns could be nice and in this case Morse could be
> one such pattern, but if that means it's limited to userland to configure
> it then it sadly voids all of its benefits.
> 
> Last, as I showed on my previous mail in this thread, the Morse encoding
> can be brought to 36 bytes, which is much less than even the registration
> code needed to feed it. At some point we need to focus on code efficiency
> and doing things right instead of how too many layers look like from far
> away. While I don't need Morse right now I consider it as a nice addition
> I would definitely enable by default on all my boards if it helps provide
> details about what prevents my system from booting or what just killed it.

I'm with Willy here, this can be some very tiny (I was waiting for
people to optimize it) code that will be smaller overall than a generic
"pattern trigger" kernel module + userspace implementation.  So I vote
for it to be merged this way.  Or, to appease everyone, why not turn
this code into a "generic" pattern trigger with the tiny addition of the
morse code interface if wanted?

thanks,

greg k-h

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Andy Shevchenko]

On Wed, Jul 4, 2018 at 5:41 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Tue, Jul 03, 2018 at 09:43:06PM +0300, Andy Shevchenko wrote:

> Well, in this case it's even possible to go further and avoid storing
> 36 strings. Indeed, no representation is longer than 5 symbols, so you
> can use 5 bits for the encoding (0=".", 1="-") and 3 bits for the
> length, it gives you a single byte per character instead of a pointer
> to a string plus 6 chars. Then in order to make it readable, 5 macros
> can be provided to emit the code :
>

Even further something like

#define MORSE_PACK(len, code)   ((code << 3) | len)

> #define MORSE1(a,b)       (1 | ((a)<<3))
> #define MORSE2(a,b)       (2 | ((a)<<3)|((b)<<4))
> #define MORSE3(a,b,c)     (3 | ((a)<<3)|((b)<<4)|((c)<<5))
> #define MORSE4(a,b,c,d)   (4 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6))
> #define MORSE5(a,b,c,d,e) (5 | ((a)<<3)|((b)<<4)|((c)<<5)|((d)<<6)|((e)<<7))
>
> Then all chars may be defined like this :
>
>     ['a'] = MORSE2(0,1),
>     ['b'] = MORSE4(1,0,0,0),
>     ['c'] = MORSE4(1,0,1,0),
>     ['d'] = MORSE3(1,0,0),
>     ['e'] = MORSE1(0),
>     ...
>
> and when processing these :
>
>     code = morse_table[tolower(c)];
>     code_len = code & 7;
>     code >>= 3;
>
>     while (code_len) {
>        if (code & 1)
>            emit_long();
>        else
>            emit_short();
>        code >>= 1;
>        code_len--;
>     }

Nice!

> In this case it could even cover the whole ASCII table at once since it's
> not certain that the saved bytes compensate for the extra code and alignment
> used to save them :-)
>
> Note that I'm not suggesting that it is required to proceed like this, but
> I think it makes the whole code more compact, which aligns with the purpose
> of focusing on embedded devices.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Pavel Machek]

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

Hi!

> On Wed, Jul 04, 2018 at 08:53:05AM +0200, Pavel Machek wrote:
> > As I stated before, I don't think morse encoder belongs in kernel.
> 
> On the opposite, I think that the kernel needs to be a bit more autonomous
> when it comes to reporting its own issues. Being able to report a panic
> when userland cannot be accessed for example is the reason why we've seen
> various features such as blinking keyboard LEDs for this.

Being able to report panics by blinking would be nice... but proposed
patch does NOT do that.

> > LED pattern trigger should be merged, instead.
> 
> Well, just like we have LED and LED triggers in the kernel, I think having
> a generic way to use patterns could be nice and in this case Morse could be
> one such pattern, but if that means it's limited to userland to configure
> it then it sadly voids all of its benefits.

Proposed patch is already limited to configuration from userland... so
it does not have any benefits.

If special "panic blinking" mode is wanted -- I guess that would make
sense.

But

a) trigger may not be right infrastructure for that; triggers need
quite a lot kernel to be working, as they run in separate threads --
panic blinking probably should be mdelay / brightness set / mdelay,
and probably limited to LEDs that can be accessed without sleeping.

b) we may want trigger to be used for something else (Caps lock? HDD
activity?)  when not panicked. Thus, again, trigger is not exactly
suitable. (It might make sense to blink many/all LEDs simultaneously
to make it super obvious to the user).

									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 v2] leds: ledtrig-morse: send out morse code

[Jacek Anaszewski]

Hi Pavel,

On 07/04/2018 08:53 AM, Pavel Machek wrote:
> On Tue 2018-07-03 17:53:28, Andreas Klinger wrote:
>> Send out a morse code by using LEDs.
>>
>> This is useful especially on embedded systems without displays to tell the
>> user about error conditions and status information.
>>
>> The trigger will be called "morse"
>>
>> The string to be send is written into the file morse_string and sent out
>> with a workqueue. Supported are letters and digits.
>>
>> With the file dot_unit the minimal time unit can be adjusted in
>> milliseconds.
>>
>> Also add documentation for the morse led trigger
>>
>> Thanks to Greg and Geert for suggesting improvements
>>
>> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> 
> As I stated before, I don't think morse encoder belongs in kernel.
> 
> LED pattern trigger should be merged, instead.

We've already agreed in [0] upon pattern sysfs file, and related
patch set [1] is just to be merged.

[0] https://lkml.org/lkml/2018/5/12/142
[1] https://patchwork.kernel.org/patch/10495595/

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Jacek Anaszewski]

Hi All,

On 07/04/2018 08:19 PM, Pavel Machek wrote:
> Hi!
> 
>> On Wed, Jul 04, 2018 at 08:53:05AM +0200, Pavel Machek wrote:
>>> As I stated before, I don't think morse encoder belongs in kernel.
>>
>> On the opposite, I think that the kernel needs to be a bit more autonomous
>> when it comes to reporting its own issues. Being able to report a panic
>> when userland cannot be accessed for example is the reason why we've seen
>> various features such as blinking keyboard LEDs for this.
> 
> Being able to report panics by blinking would be nice... but proposed
> patch does NOT do that.

Actually we do have already drivers/leds/trigger/ledtrig-panic.c.

>>> LED pattern trigger should be merged, instead.
>>
>> Well, just like we have LED and LED triggers in the kernel, I think having
>> a generic way to use patterns could be nice and in this case Morse could be
>> one such pattern, but if that means it's limited to userland to configure
>> it then it sadly voids all of its benefits.
> 
> Proposed patch is already limited to configuration from userland... so
> it does not have any benefits.

We could turn this piece of code into a lib and make it usable by
both kernel/panic.c (similar to panic_blink) as well by
the ledtrig-morse.c.

> If special "panic blinking" mode is wanted -- I guess that would make
> sense.
> 
> But
> 
> a) trigger may not be right infrastructure for that; triggers need
> quite a lot kernel to be working, as they run in separate threads --
> panic blinking probably should be mdelay / brightness set / mdelay,
> and probably limited to LEDs that can be accessed without sleeping.
> 
> b) we may want trigger to be used for something else (Caps lock? HDD
> activity?)  when not panicked. Thus, again, trigger is not exactly
> suitable. (It might make sense to blink many/all LEDs simultaneously
> to make it super obvious to the user).
> 
> 									Pavel
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Pavel Machek]

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

Hi!

> >LED pattern trigger should be merged, instead.
> 
> We've already agreed in [0] upon pattern sysfs file, and related
> patch set [1] is just to be merged.
> 
> [0] https://lkml.org/lkml/2018/5/12/142
> [1] https://patchwork.kernel.org/patch/10495595/

Well, [1] is for LEDs that can do patterns in hardware. I'm aware of
that work.

I still believe that we want to do "trigger" that can do the same on
hardware that is not so lucky.

Best regards,
									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 v2] leds: ledtrig-morse: send out morse code

[David Laight]

...
> > Well, just like we have LED and LED triggers in the kernel, I think having
> > a generic way to use patterns could be nice and in this case Morse could be
> > one such pattern, but if that means it's limited to userland to configure
> > it then it sadly voids all of its benefits.

I've been involved in a system that used morse for short error messages.
Unless you actually know morse it is completely hopeless.

With two leds next to each other you should be able to generate
a lot of easily identifiable patterns by repeating separate 4-bit
patterns (in sync) on both leds (at about 2Hz update).

	David

Re: [PATCH v2] leds: ledtrig-morse: send out morse code

[Geert Uytterhoeven]

Hi Willy,

On Wed, Jul 4, 2018 at 4:41 AM Willy Tarreau <w@1wt.eu> wrote:
> On Tue, Jul 03, 2018 at 09:43:06PM +0300, Andy Shevchenko wrote:
> Well, in this case it's even possible to go further and avoid storing
> 36 strings. Indeed, no representation is longer than 5 symbols, so you
> can use 5 bits for the encoding (0=".", 1="-") and 3 bits for the
> length, it gives you a single byte per character instead of a pointer
> to a string plus 6 chars. Then in order to make it readable, 5 macros
> can be provided to emit the code :

And using the scheme from
https://plus.google.com/u/0/117536210417097546339/posts/hvctn17WUZu
you can store up to 7 symbols in a single byte, which you need when going
beyond plain alphanumeric:

-0111111
--011111
---01111
----0111
-----011
------01
-------0

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds