mcci-catena/arduino-lmic

Errors in documentation

lnlp opened this issue · 4 comments

lnlp commented

I found the following errors and inconsistencies in the .docx/.pdf documentation:

ISSUE 1:

2.2.15 ostime_t us2osticks(s4_t sec)

should read:

2.2.15 ostime_t sec2osticks(s4_t sec)

ISSUE 2:

2.3.4 void **onEvent(ev_t ev)

"In any case, this interface is better handled by registering an event function at run time. See the discussion of LMIC_registerEventCb(), below."

An explanation for why it is better ('in any case') is missing. Why is it better?

'In any case better' but use of LMIC_registerEventCb() currently always requires the following to be defined (to disable the legacy onEvent() event handler):

    #define LMIC_ENABLE_onEvent 0

This dependency / requirement is undesired and can be prevented (see #433).
In fact it is currently not even documented for LMIC_registerEventCb() (see issue 8).

ISSUE 3:

2.5.12 void LMIC_setTxData_strict()

Lacks a clear description of what makes LMIC_setTxData_strict() different from LMIC_setTxData().
Such description should be put at the start.
The first paragraph in 2.5.12 is just a repetition of the first paragraph of 2.5.11 (but uses different words which makes it more difficult to compare). The repetition is redundant and unnecessary. Instead the first paragraph should be replaced with a description of what it is different from LMIC_setTxData().

"This API is preferred for new applications, for two reasons...." This is an advice to use the strict version but should not be a replacement for the lacking description at the start.

ISSUE 4:

2.5.15 lmic_tx_error_t ... the following values may be returned.

Description for value -3 should read "for any enabled data rate" instead of "for the current data rate".

ISSUE 5:

2.5.15 and 2.5.16

Documentation for LMIC_sendWithCallback() and LMIC_sendWithCallback_strict() does not make clear if the Callback is a replacement for the EV_TXCOMPLETE event or not.
I.e. will an EV_TXCOMPLETE event (if successful) also be generated or will only the callback be called and will te EV_TXCOMPLETE event not be generated?
(Not clear because it is not explicitly mentioned and because of inconsistency between descriptions in tables in 2.5.15 and 2.5.16).

ISSUE 6:

2.5.15 and 2.5.16

Depending on what is described at Issue 5 (will EV_TXCOMPLETE be generated or not?):

  • Either the descriptions in the table in 2.5.15 should mention EV_TXCOMPLETE,
  • or, EV_TXCOMPLETE should be removed from the description in the table in 2.5.16.

ISSUE 7:

2.5.15 lmic_tx_error_t ... the following values may be returned.

Table with lmic_tx_error_t values is already listed in section 2.5.13.
Repeating the table in 2.5.13 gives the impression that it may be different from the table in 2.5.13 which it actually is not. Comparing the descriptions in both tables shows they are not textually identical but they appear to mean exactly the same.
I would like to suggest to leave out the table in 2.5.15 and add a reference to the table 2.5.13 instead.
If I'm correct the only difference between 2.5.13 and 2.5.15 is the addition of a callback but no differences in the return values and their meaning.

ISSUE 8:

2.5.19 void LMIC_registerEventCb(...)

Lacks mentioning dependency / requirement:

    #define LMIC_ENABLE_onEvent 0

ISSUE 9:

2.5.19 void LMIC_registerEventCb(...)

The EV_LINK_DEAD section is listed twice.

Suggestion:

  • Adding a revision history chapter to the document would be useful.

Thanks for your careful review.

Issue 1. Agreed.
Issue 2. Docs can be improved, but the code does not require that you supply an onEvent function. See my comments under #433.
Issue 3. Agreed
Issue 4. Agreed
Issue 5. Agreed, will try to clarify.
Issue 6. Agreed.
Issue 7. Yes, we should avoid duplication of that kind.
Issue 8. See my comments under #433. It is not required. The only thing additional if you #define it is that a few bytes are saved.
Issue 9. Agreed.

Suggestion. Agreed; I have tools to automatically generate a decent Word redline, and I'll do that once the document stabilizes.

I can't find the duplication of EV_LINK_DEAD.

Hi @lnlp -- because of the delay in making the release due to #635, there's a chance to review the changes, if you have time...

lnlp commented

Hi @terrillmoore,
I'll have to skip it at the moment, sorry.