#319 'pre' and 'post' in *.conf is ignored if no 'pre_data', 'post_data'

[Alec Leamas]

In a configuration file for a remote control, I use 'post' to send a pulse
(30 us) and a space (460 us) after the data, then ptrail for a trailing pulse.
The 'post' cannot be part of 'data' as the space of 'zero' and 'one' is 650 us
and 830 us (not 460 us). The pulse and space configured in 'post' is, however,
not sent, i.e., the 'post' directive is silently ignored.

Looking into the source code I found the issue in lib/transmit.c:
'post' (i.e., a pulse and a space of specific lengths) is only sent if also
'post_data' (i.e., a sequence of 'zero' and 'one') is configured. The function
send_post(remote) checks if has_post(remote) is true; this is only true if
remote->post_data_bits > 0.

Same applies to 'pre' and 'pre_data'.

This limitation is not described in the documentation
(see http://www.lirc.org/html/lircd.conf.html). Thus, I suggest to either
update the documentation or fix the code. I prefer the latter as I see no logic
why a directive should be ignored. I attach a patch.

For my specific use case, I have a workaround: I replace 'ptrail' by a single
bit 'post_data'. This only works because the 'ptrail' pulse happens to have
the same duration as the pulse of 'one'. Thus, I now have 'post_data' and
therefore 'post' is no longer ignored. My workaround results in having two
spaces at the end (the space of the 'one' from 'post_data' and the 'gap'); this
does no harm.

I have not found a generic workaround that can be used if the trailing pulse
has a different duration than 'one' and 'zero', nor for repeat sequences.

[Alec Leamas]

First of all, since the code and the docs don't match we need to fix one
of those.

The question then becomes why you have stumbled over this. Do you have a
usecase where this bug (be it in the docs or in the code) shows up?

[Alec Leamas]

This tiicket is a tracker for downstream bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=890374

[Al_]

Two .conf files to further clarify the usecase (see first paragraph of the original bug report). zehnderheating.conf does not send the 'post' pulse and space. zehnderheatingworkaround.conf is a workaround (that is possible as the trailing pulse happens to have the same duration as data pulses).

[Bengt Martensson]

I previously wrote on the mailing list

I would strongly advice against "fixing bugs" like that.

This is what the man-page of lircd.conf says:

LIRC  was  designed  to  collect IR data and save it in a private,
compact, yet human readable format with the purpose of being able to
re-transmit (or re-recognize) these signals. It was not designed
with the goal of providing a well documented and tested configuration
file format that could be used e.g., to generate arbitrary IR signals
or to convert them to other formats. The configuration  file
 hould thus not be considered a public interface to LIRC.

This  manpage  should be understood with this in mind. It was
authored by reading the code of LIRC, and does not constitute an
authoritative specification of the behavior of LIRC and its
configuration
   file.  ...

Lirc is not a well defined language with an associated rendering engine.
Its core was written almost 20 years ago by people who are no longer active. Probably it is fair to say that no-one today really understands the code. The documentation of the lircd.conf keywords was written many years later by other persons, first by the WinLirc people and then by Alec, "by reading the code of LIRC".

Also keep in mind that Lirc, in different guises, is used fairly widely. If changing the current version, we have different versions that render the same file differently. How do you make clear that the rendering of all present files is not changed? Or, it they are affected by the "bug", should they render differently?

For these reasons, I think the Lirc rendering engine should not be changed in a non-trivial way, like "bug fixes".

As a principle, I still think it is important not to change an old, relatively spread system, "just because" you have found some quirks. But in this case, the current behavior is highly bizarre: Under some circumstances, data (pre and post) are quietly ignored (instead of thowing a warning or an error). It is highly unlikely that any "old" behavior depends on this, but of course this has to be checked. (Are there any files in lirc-remotes that defines post/pre without post_data/pre_data? This can and should be easily checked.)

After this has been checked, I would approve the fix (to the sources).

For my specific use case, I have a workaround: I replace 'ptrail' by a single
bit 'post_data'.

The most natural work-aroung would be to use raw codes, IMHO.

[Al_]

Excellent idea to check the publicly available remotes for use of pre and post.

I downloaded today a snapshot from https://sourceforge.net/p/lirc-remotes/code/ci/master/tree/remotes/ , then run the following two grep commands to search for pre and post with a pulse length >0 (in the square brackets is a space and a literal tab):

grep -A 5 -B 5 --recursive --extended-regexp --regexp='pre[   ]+[0-9][1-9]' ./
grep -A 5 -B 5 --recursive --extended-regexp --regexp='post[   ]+[0-9][1-9]' ./

There is no output from the second grep command; in all remote definitions returned by the first grep is also a pre_data_bits >0.

Thus, a fix to the source should not break the existing publicly available remote configurations.

[Christoph Bartelmus]

"pre/post" was originally meant to be a separator between "data" and "pre_data/post_data". If pre_data/post_data is empty of course no separator would be required. Please update the documentation accordingly. What initially was intended by the OP could probably be implemented using the "foot" field.

[Al_]

The 'foot' field does in my opinion not allow the behaviour possible with 'post': 'data' ends with a space; the required bahaviour is a following pulse space pulse (and a long gap until the next IR key)
As per http://www.lirc.org/html/lircd.conf.html, 'data' is only followed (if we cannot use 'post' and 'post_data') by ptrail (only a pulse), foot (pulse space) and gap (only a space) => pulse pulse space space. I.e., there is no option for a space between two pulses.

[Alec Leamas]

First, I have updated the manpage (master) according to Christoph's remark: [60ed3b].

Secondly, I see nothing wrong in the basic usescase here. So, the question then becomes if we can support it without breaking any eggs - changing these things should certainly not be taken lightly. That said, are there any specific reasons against applying the patch from the downstream bug?

Of course, applying this must be tested using the regression tests. I might be able to find some time for this if this is the final roadblock.

EDIT: Multiple embarrasing typos, linking.

[Christoph Bartelmus]

The specific reason is that it is not required.
The usecase can perfectly be implemented using ptrail and foot (foot is sent as "space pulse"). pre and post are separators between two compontents. If one component does not exist, there is no need for a separator. Please add a warning as suggested by Bengt.
Cheers, Christoph

[Bengt Martensson]

A few comments:

First, I have updated the manpage (master) ...

Sorry, even with the new version I do not think it is clear at all that post/pre is ignored if post_data/pre_data is empty. Secondly, consider also to change

 |header|plead|pre data|pre|data|post|post data|ptrail|foot|gap|

Secondly, I see nothing wrong in the basic usescase here.

I do. As I quoted, the exact semantics of the keywords is not considered public API, so strictly speaking, this is not a supported use case. If a rendering engine is required, use a such. Having said that, already documenting the keywords makes users want to use them. Fixing problems might still be considered.

If pre_data/post_data is empty of course no separator would be required.

It is bad programming to accept a parameter, and then quietly ignoring it. If pre/post is prohibited without pre_data/post_data, then the program should detect it and barf.

Summing up. I think more arguments speak in favor of the change than against. Unless if some comes up with something that really breaks...

[Sean Young]

I've tested this and fixing this doesn't break any lircd.conf file I can have. I think it makes sense to fix it - it's just very unexpected, I think.

[Sean Young]

Actually, I think the code is buggy. For receive, the pre is parsed if there are no pre_bits, same for post without any post bits.

So, transmit is buggy compared to receive. This should be fixed.

[Sean Young]

Fixed in commit 52550a56db638d521ddcb5ba6bc20389acf5f178

[Sean Young]

Actually it turns out, receive did not implement this. I've fixed the receive side in commit 1dc29314729f193b148d10f4ac575582d303ab24