leds: ledtrig-morse: send out morse code

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

drivers/leds/trigger/ledtrig-morse.c new file +4 -0

@@ -0,0 +1,298 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ledtrig-morse: LED Morse Trigger
+ *

+#define MORSE_DOT_UNIT_DEFAULT 500

+#define MORSE_TELEGRAM_SIZE 100

drivers/leds/trigger/ledtrig-morse.c new file +7 -0

+
+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;

drivers/leds/trigger/ledtrig-morse.c new file +26 -0

+       {'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', "--.."},

+ {0, NULL},

drivers/leds/trigger/ledtrig-morse.c new file +31 -0

+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);
+       }
+}

drivers/leds/trigger/ledtrig-morse.c new file +22 -0

+
+       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);

drivers/leds/trigger/ledtrig-morse.c new file +25 -0

+
+       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;

drivers/leds/trigger/ledtrig-morse.c new file +13 -0

+
+       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:

drivers/leds/trigger/ledtrig-morse.c new file +4 -0

+
+               cancel_work_sync(&data->work);
+
+               device_remove_file(led_cdev->dev, &dev_attr_morse_string);

drivers/leds/trigger/ledtrig-morse.c line 44

Quoted 7 lines

drivers/leds/trigger/ledtrig-morse.c new file +6 -0

+struct morse_char {
+       char    c;
+       char    *z;
+};
+
 
+static struct morse_char morse_table[] = {

const ?

Quoted 26 lines

drivers/leds/trigger/ledtrig-morse.c new file +26 -0

+       {'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', "--.."},

drivers/leds/trigger/ledtrig-morse.c line 80

Quoted 10 lines

drivers/leds/trigger/ledtrig-morse.c new file +10 -0

+       {'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;

Quoted 1 line

+ {0, NULL},

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

Quoted 20 lines · Andy Shevchenko

drivers/leds/trigger/ledtrig-morse.c line 46

Quoted 2 lines

drivers/leds/trigger/ledtrig-morse.c new file +2 -0

+       {'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;

Quoted 1 line

+ {0, NULL},

Quoted 1 line

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),

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 :

#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.

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 :

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 <[email protected]>

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

LED pattern trigger should be merged, instead.

Hi Pavel,

Quoted 1 line · Pavel Machek

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.

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 1 line

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.

Hi!

Quoted 3 lines · Pavel Machek

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 6 lines

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 15 lines · 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

Quoted 3 lines

Thanks to Greg and Geert for suggesting improvements

Signed-off-by Andreas Klinger <[email protected]>

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

LED pattern trigger should be merged, instead.

Quoted 1 line

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/