modem: fixes and improvements

[tomi-font]

See details in commit messages.

[tomi-font]

Ping @bjarki-trackunit to make sure this does not fall into oblivion.

[bjarki-andreasen]

The intended behavior is that the user reacts to the PIPE_CLOSED event, and reopens the pipe is required. This allows the user to do any prerequisite work like resetting whatever the pipe was connected to, or ignoring the event if the closing is either expected or inconsequential (like the modem_cellular.c driver where the pipe is closed when the modem switches from AT to CMUX mode as the UART RX goes low for a period)

[tomi-font]

I see. However in (at least) the case of the CMUX module being attached to the backend, it is the one to receive the pipe events. Currently it doesn't forward the events to the user nor react to the pipe closed event.

I currently see two different ways out of this:

1. Let the user pass a callback so that events pertaining to the pipe the module is attached to can be received and reacted to.
2. Make whether the pipe is automatically re-opened configurable (at build or run time).

What do you think?

[bjarki-andreasen]

CMUX should disconnect if the pipe is closed, which will also close every DLCI channel, propagating the closed event :) So essentially option 1

[tomi-font]

I did some more testing to reproduce the issue that I was fixing here, and turns out that I remembered wrong.
It can happen when using the PPP module directly plugged in the UART backend. No CMUX involved.

So it's the same story there, except there is no pipe shared between the PPP module and the user, so what you proposed doesn't apply.

Do you have an opinion on how it should be done there?

[bjarki-andreasen]

Nope, I have no clear way to justify adding error handling to the PPP module that could not be applied to the CMUX module. I will review the addition of the auto recovery here:

We can not use

k_sleep(K_MSEC(100));

in a work item, as this blocks the work queue. We will need to add an additional delayable work item which waits for those 100ms.

When that work item is started, it must use modem_pipe_open_async() as modem_pipe_open() can block the system workqueue, which becomes a problem if the UART driver also uses it, as this will deadlock the workqueue until modem_pipe_open() times out

This will mean that we can end up in a race condition where the user wants to close the pipe, while we are waiting to try and reopen it, so make sure to cancel the work item in the modem_pipe_close implementation :)

[bjarki-andreasen]

Of course, we also have the option op implementing the reopening in the modules using the pipes, so CMUX would reopen if it is connected, and L2 PPP would reopen if it is started, this would scale to all backends (UART ISR, TTY, UART ASYNC etc.)

[tomi-font]

Thanks for the explanation regarding the work item and the synchronous opening.

I will leave the automatic recovery out of this PR so that the other changes can get in, but I am still planning to do something about it. Just have to agree on how exactly.

Nope, I have no clear way to justify adding error handling to the PPP module that could not be applied to the CMUX module.

I am not sure I quite understood this. Did you imply that the solution should apply to both the PPP and CMUX modules, so it shouldn't be specific to the PPP module? Although in what you propose further down, if I understand correctly each module would handle the reopening itself, so in that sense it would be specific to each module.

Of course, we also have the option op implementing the reopening in the modules using the pipes, so CMUX would reopen if it is connected, and L2 would reconnect if it is started, this would scale to all backends (UART ISR, TTY, UART ASYNC etc.)

Here what are you referring to exactly with L2?

[bjarki-andreasen]

If we extend the modem PPP (L2) module to always automatically reopen the pipe, we should extend the same logic to the modem CMUX as well.

We can make the backends "volatile", simply closing if they encounter an internal error, and make the user reopen it if that is desired. If this is all we are doing internally within the backend's anyway, we might as well just propagate the issue to the user with the closed event.

We can also make the backends "stronger", allowing them to do whatever they need internally to recover. This will be simpler from a user's perspective. The pipes would not close themselves, they would simply try to recover until it either succeeds or the pipe is closed due to some higher level timeout.

[bjarki-andreasen]

I would prefer this approach:

static bool modem_ppp_process_is_byte_expected(uint8_t byte, uint8_t expected_byte)
        if (byte == expected_byte) {
	        return true;
        }

        LOG_DBG("Dropping byte 0x%2hhx because 0x%2hhx was expected.", byte, expected_byte);
	return false;
}

...

{
	switch (ppp->receive_state) {
	case MODEM_PPP_RECEIVE_STATE_HDR_SOF:
                if (modem_ppp_process_is_byte_expected(byte, MODEM_PPP_CODE_DELIMITER)) {
                        ppp->receive_state = MODEM_PPP_RECEIVE_STATE_HDR_FF;
                }
		break;

As this is easier to follow than hiding the new state within the helper, and using ++ instead of setting it directly. It will take up a few more bytes of ROM, but in this case the readability is more important IMO.

[tomi-font]

ping @bjarki-trackunit @ycsin @rerickson1 @fabiobaltieri @bbilas