Modem cmux DCE #65955

jheiskan81 · 2023-11-30T08:47:22Z

Modem cmux update

Added missing command and message handling for use existing modem CMUX for DCE role.
DCE CMUX connection can be now initialized from DTE side.

Modem test update

Added possibility for link 2 modem backend mock instance for validating P2P communication without any data stubbing.
Added unite test for for validate modem CMUX DTE & DCE role communication together.

bjarki-andreasen · 2023-11-30T09:26:50Z

Initial review is really promising, I will find time to review it in detail :)

bjarki-andreasen

Really like it, I want to discuss the flow control API, see my comment :)

bjarki-andreasen · 2023-12-01T12:21:13Z

include/zephyr/modem/cmux.h

+ * @return -EPIPE CMUX control channel is not open.
+ * @return -EALREADY Ongoing flow control request.
+ */
+int modem_cmux_flow_ctrl_set_asynch(struct modem_cmux *cmux, bool state);


Could you describe the usecase for this API? The CMUX instance has an RX buffer, if the RX buffer nears full, the CMUX instance should automatically disable flow IMO :)

Main reason for this new API was that I try to test similar functionality than your unit test with stubbed. I was also wondering that 3GGP CMUX define flow control for / DLCI channel now all channels are active or disabled. Now both end could call it. It will disable or enable flow control at caller side and remote side.

I'm really not sold on the manual API...

The CMUX instance knows the state of all buffers, its TX buffer, the CMUX RX buffer, and every DLCI RX buffer. CMUX defines two types of flow control, the flow control messages FCOFF and FCON, and individual channel flow control through MSC. We know we need to disable flow control for all channels if the CMUX RX buffer nears full, and we know the same for each DLCI RX buffer, so we really should be handling it automatically from within the CMUX instance.

Flow control should probably be its own PR, or at least its own commit, and should be handled internally. Its the simplest and most scalable solution.

tests/subsys/modem/modem_cmux_pair/testcase.yaml

tests/subsys/modem/modem_cmux_pair/src/main.c

jheiskan81 · 2023-12-01T13:48:26Z

I update test with your comment's and added some extra verfify for test_modem_cmux_disconnect_connect_sync() and test_modem_cmux_dlci_close_open_sync() test cases for DCE.

bjarki-andreasen

I would suggest you move flow control out into its own PR, it should be handled internally by CMUX. The DCE additions and test case look excellent :)

bjarki-andreasen · 2023-12-04T21:34:57Z

include/zephyr/modem/cmux.h

+ * @return -EPIPE CMUX control channel is not open.
+ * @return -EALREADY Ongoing flow control request.
+ */
+int modem_cmux_flow_ctrl_set_asynch(struct modem_cmux *cmux, bool state);


I'm really not sold on the manual API...

The CMUX instance knows the state of all buffers, its TX buffer, the CMUX RX buffer, and every DLCI RX buffer. CMUX defines two types of flow control, the flow control messages FCOFF and FCON, and individual channel flow control through MSC. We know we need to disable flow control for all channels if the CMUX RX buffer nears full, and we know the same for each DLCI RX buffer, so we really should be handling it automatically from within the CMUX instance.

Flow control should probably be its own PR, or at least its own commit, and should be handled internally. Its the simplest and most scalable solution.

jheiskan81 · 2023-12-05T05:21:40Z

@bjarki-trackunit Thanks for review. I remove Flow Control API and functionality. I have updated PR.

bjarki-andreasen · 2023-12-05T08:40:30Z

Nice, I will re-review later today

bjarki-andreasen · 2023-12-06T17:54:58Z

I will spend some time tomorrow as well, I'm fairly certain the changes to fcon and fcoff are not necessary, I will need to look more into it locally :)

jheiskan81 · 2023-12-07T11:23:17Z

@bjarki-trackunit Please review. I have put flow control to original format back.

bjarki-andreasen

Tested the changes locally and it behaves as expected :) There is one spelling mistake, when that is fixed we are good to go :)

bjarki-andreasen · 2023-12-07T16:31:59Z

subsys/modem/modem_cmux.c

@@ -435,6 +446,40 @@ static void modem_cmux_on_control_frame_uih(struct modem_cmux *cmux)
 	}
 }

+static void modem_cmux_conect_response_transmit(struct modem_cmux *cmux)


Should be _connect_

I will be fix this after my bicycle ride. Thanks

Added missing command and message handling for use existing modem cmux for DCE role. DCE CMUX connection can be now initialized from DTE side. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

Added possibility for link 2 modem mock instance for validating P2P communication without any data stubbing. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

Added unite test for for validate modem CMUX DTE & DCE role communication together. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

bjarki-andreasen

Nice work :)

zephyrbot added the area: Modem label Nov 30, 2023

zephyrbot requested a review from bjarki-andreasen November 30, 2023 08:47

zephyrbot assigned bjarki-andreasen Nov 30, 2023

jheiskan81 force-pushed the modem_cmux_dce branch from c20857f to d34d2cf Compare November 30, 2023 09:55

bjarki-andreasen requested changes Dec 1, 2023

View reviewed changes

jheiskan81 force-pushed the modem_cmux_dce branch 2 times, most recently from dc52acf to b77808f Compare December 1, 2023 13:46

jheiskan81 requested a review from bjarki-andreasen December 1, 2023 13:46

jheiskan81 force-pushed the modem_cmux_dce branch 2 times, most recently from e21f323 to 1b54fc7 Compare December 1, 2023 18:01

bjarki-andreasen requested changes Dec 4, 2023

View reviewed changes

jheiskan81 force-pushed the modem_cmux_dce branch from 1b54fc7 to b7ba18f Compare December 5, 2023 05:16

jheiskan81 requested a review from bjarki-andreasen December 5, 2023 05:20

jheiskan81 force-pushed the modem_cmux_dce branch from b7ba18f to b828d0e Compare December 7, 2023 11:22

bjarki-andreasen requested changes Dec 7, 2023

View reviewed changes

Juha Heiskanen added 3 commits December 7, 2023 23:14

modem: modem_cmux: Support modem CMUX DCE role

9ccb2cc

Added missing command and message handling for use existing modem cmux for DCE role. DCE CMUX connection can be now initialized from DTE side. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

tests: modem: Modem backend mock update

6ce37e4

Added possibility for link 2 modem mock instance for validating P2P communication without any data stubbing. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

tests: mode: Modem CMUX DTE / DCE test

f4c5944

Added unite test for for validate modem CMUX DTE & DCE role communication together. Signed-off-by: Juha Heiskanen <juha.heiskanen@nordicsemi.no>

jheiskan81 force-pushed the modem_cmux_dce branch from b828d0e to f4c5944 Compare December 7, 2023 21:45

jheiskan81 requested a review from bjarki-andreasen December 7, 2023 21:46

bjarki-andreasen approved these changes Dec 8, 2023

View reviewed changes

bjarki-andreasen requested review from fabiobaltieri, rerickson1 and ycsin December 8, 2023 08:35

fabiobaltieri approved these changes Dec 8, 2023

View reviewed changes

rerickson1 approved these changes Dec 8, 2023

View reviewed changes

fabiobaltieri merged commit 34d4e50 into zephyrproject-rtos:main Dec 8, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modem cmux DCE #65955

Modem cmux DCE #65955

jheiskan81 commented Nov 30, 2023 •

edited

Loading

bjarki-andreasen commented Nov 30, 2023

bjarki-andreasen left a comment

bjarki-andreasen Dec 1, 2023

jheiskan81 Dec 1, 2023

bjarki-andreasen Dec 4, 2023 •

edited

Loading

jheiskan81 commented Dec 1, 2023

bjarki-andreasen left a comment

bjarki-andreasen Dec 4, 2023 •

edited

Loading

jheiskan81 commented Dec 5, 2023

bjarki-andreasen commented Dec 5, 2023

bjarki-andreasen commented Dec 6, 2023

jheiskan81 commented Dec 7, 2023

bjarki-andreasen left a comment

bjarki-andreasen Dec 7, 2023

jheiskan81 Dec 7, 2023

bjarki-andreasen left a comment

Modem cmux DCE #65955

Modem cmux DCE #65955

Conversation

jheiskan81 commented Nov 30, 2023 • edited Loading

bjarki-andreasen commented Nov 30, 2023

bjarki-andreasen left a comment

Choose a reason for hiding this comment

bjarki-andreasen Dec 1, 2023

Choose a reason for hiding this comment

jheiskan81 Dec 1, 2023

Choose a reason for hiding this comment

bjarki-andreasen Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

jheiskan81 commented Dec 1, 2023

bjarki-andreasen left a comment

Choose a reason for hiding this comment

bjarki-andreasen Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

jheiskan81 commented Dec 5, 2023

bjarki-andreasen commented Dec 5, 2023

bjarki-andreasen commented Dec 6, 2023

jheiskan81 commented Dec 7, 2023

bjarki-andreasen left a comment

Choose a reason for hiding this comment

bjarki-andreasen Dec 7, 2023

Choose a reason for hiding this comment

jheiskan81 Dec 7, 2023

Choose a reason for hiding this comment

bjarki-andreasen left a comment

Choose a reason for hiding this comment

jheiskan81 commented Nov 30, 2023 •

edited

Loading

bjarki-andreasen Dec 4, 2023 •

edited

Loading

bjarki-andreasen Dec 4, 2023 •

edited

Loading