Florian Eckert wrote on 2023-10-30 09:15:
+ /* The rx/tx handling must come after the evaluation of TIOCM_*,
+ * since the display for rx/tx has priority
+ */
+ if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) ||
+ test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger)) {
+ ret = tty_get_icount(trigger_data->tty, &icount);
+ if (ret) {
+ dev_info(trigger_data->tty->dev, "Failed to get icount, stopped
polling\n");
+ mutex_unlock(&trigger_data->mutex);
+ return;
+ }
+
+ if (test_bit(TRIGGER_TTY_RX, &trigger_data->ttytrigger) &&
+ (icount.tx != trigger_data->tx)) {
You check for TRIGGER_TTY_RX and then compare icount.tx, is that
correct?
I would say this is correct. At first I check if the tx path should be
evaluated
and if this is correct I check if there was a tx transmission during
the last run.
No, you check if the *RX* path should be evaluated! On the bright side:
this is
fixed in the new patch set.
+ trigger_data->tx = icount.tx;
+ state = TTY_LED_BLINK;
+ }
+
+ if (test_bit(TRIGGER_TTY_TX, &trigger_data->ttytrigger) &&
+ (icount.rx != trigger_data->rx)) {
You check for TRIGGER_TTY_TX and then compare icount.rx, is that
correct?
I would say this is correct. At first I check if the rx path should be
evaluated
and if this is correct I check if there was a rx transmission during
the last run.
Same difference.
+ trigger_data->rx = icount.rx;
+ state = TTY_LED_BLINK;
+ }
}
- if (icount.rx != trigger_data->rx ||
- icount.tx != trigger_data->tx) {
- unsigned long interval = LEDTRIG_TTY_INTERVAL;
+ current_brightness = led_cdev->brightness;
+ if (current_brightness)
+ led_cdev->blink_brightness = current_brightness;
+ if (!led_cdev->blink_brightness)
+ led_cdev->blink_brightness = led_cdev->max_brightness;
Is it OK to override the chosen brightness here?
In my setup my brightness in the sysfs path of the LED ist set to '0'.
Even though the tty trigger was configured correctly the LED was not
turned on. If I set max_brightness in this path the LED works
correctly.
I would check this a gain if this is still needed.
I see you've dropped this from the new patch set. Thank you.
Shouldn't the led return to the line controlled steady state?
Sorry I do not understand your question.
Set an invert variable to true if state was TTY_LED_ENABLE before it
got set
to TTY_LED_BLINK
No matter whether the LED is on or off beforehand. I understand that
the
LED is always on for the first half of the period and off for the rest
of
the period. I think that is correct and I don't need to make a
distinction
via invert here. I hope I have understood your comment correctly here.
How do interval and the frequency of ledtrig_tty_work() relate?
The work is twice as long as of the interval. So the variable
LEDTRIG_TTY_INTERVAL = 50 and the work is scheduled
LEDTRIG_TTY_INTERVAL * 2.
But that was also before my change.
This explains why you don't necessarily need to invert the blink.
If E.g. both CTS and TX are configured I would expect to see the led
turn on
once CTS actives and then blink off when something is transmitted. After
that
I expect to see the led still on because CTS is still active.
Now only because the work interval is 2*LEDTRIG_TTY_INTERVAL and the
blink
uses an interval of LEDTRIG_TTY_INTERVAL for both on and off the user
doesn't
notice any difference except maybe a bit of delay of the blink.
If either the work schedule was larger than 2*LEDTRIG_TTY_INTERVAL or
the on
interval would differ from the off interval the behaviour would differ
noticably.
This is why I recommend to use an invert variable that is set to true
when
the previous state was TTY_LED_ENABLE.
Maarten
