eclipse-threadx / netxduo

Eclipse ThreadX - NetXDuo is an advanced, industrial-grade TCP/IP network stack designed specifically for deeply embedded real-time and IoT applications
https://github.com/eclipse-threadx/rtos-docs/blob/main/rtos-docs/netx-duo/index.md
MIT License
230 stars 131 forks source link

nx_ppp cannot handle 8-bit Protocol ID in frame header #154

Closed marcin-de closed 1 year ago

marcin-de commented 1 year ago

According to RFC1661 ("PPP Encapsulation"), Protocol ID field can be coded either as one or two octets. Particularly, when Protocol ID is of type "IP Data" (0x21), it can be coded either as 0x00 0x21 or 0x21.

nx_ppp always expects two octets of Protocol ID field (lines 1479 and 1927+). Consequently, when PPP server uses one octet (as in new Thales ELS62-W modem), PPP frames with one-octet Protocol ID are discarded, thus preventing PPP driver from performing data transfer (even if PPP session is established correctly).

(This issue is copied from netx, as that repository seems to be abandoned and the issue still exists in netxduo)

wenhui-xie commented 1 year ago

@marcin-de Thanks for your report, we will look into this issue.

wenhui-xie commented 1 year ago

@marcin-de Below is the decription of PFC option: 6.5. Protocol-Field-Compression (PFC)

Description

  This Configuration Option provides a method to negotiate the
  compression of the PPP Protocol field.  By default, all
  implementations MUST transmit packets with two octet PPP Protocol
  fields.

  PPP Protocol field numbers are chosen such that some values may be
  compressed into a single octet form which is clearly
  distinguishable from the two octet form.  This Configuration
  Option is sent to inform the peer that the implementation can
  receive such single octet Protocol fields.

Our NetXDuo PPP doesn't support processing compressed data now. But from the RFC, only if NetxDuo send this PFC option to the peer, the peer can send one-octect Protocal ID to NetXDuo. Otherwise the peer must send two-octets Protocal ID. If the peer send this option to NetXDuo only means it can receive the compressed data, not means it will send the compressed data. So even if NetXDuo accepts this option, the peer should not send compressed data(one-octect Protocal ID).

This issue is similar to the ACFC option issue: https://github.com/azure-rtos/netxduo/issues/100. And it is the issue of modem side.

If you want to fix this issue locally without changing the modem, a workaround is to comment out the line 3638 and 3639 of nx_ppp.c: https://github.com/azure-rtos/netxduo/blob/master/addons/ppp/nx_ppp.c#L3638-L3639 to reject the compression options.

marcin-de commented 1 year ago

@wenhui-xie thank you for the explanation.

I will check behavior of the modem used when I disable PFC akcnowledgement and eventually raise issue to modem's manufacturer

However, both mentioned lines are there to configm sender's declaration that he is able to receive such compressed data - but if our side does not offer compression, sender should not send compressed PF even if he is able to receive it. It seems to be popular misconception about PPP options negotiation - I already have line 3639 commented out, because another modem tried to use ACFC even if I did not allow it...

marcin-de commented 1 year ago

In case you want to enhance your PPP driver to allow compressed PF, here are needed changes:

Line 1479 replaced with:

    if (packet_ptr -> nx_packet_prepend_ptr[0] & 0x01)
        protocol =  (UINT) packet_ptr -> nx_packet_prepend_ptr[0];
    else
        protocol =  (((UINT) packet_ptr -> nx_packet_prepend_ptr[0]) << 8) | ((UINT) packet_ptr -> nx_packet_prepend_ptr[1]);

and lines 1927 & 1930 replaced with:

    if ( *(packet_ptr -> nx_packet_prepend_ptr) & 0x01 ) {
        packet_ptr -> nx_packet_prepend_ptr =  packet_ptr -> nx_packet_prepend_ptr + 1;
        packet_ptr -> nx_packet_length =  packet_ptr -> nx_packet_length - 1;
    } else {
        packet_ptr -> nx_packet_prepend_ptr =  packet_ptr -> nx_packet_prepend_ptr + 2;
        packet_ptr -> nx_packet_length =  packet_ptr -> nx_packet_length - 2;
    }

IP Data is the only handled Protocol ID which may be compressed, so the above is sufficient.

wenhui-xie commented 1 year ago

@marcin-de Since these compression issues have been mentioned several times, our team is considering modifying the NetXDuo PPP. Thanks a lot for your suggestion.

bo-ms commented 1 year ago

Closing

wenhui-xie commented 1 year ago

Hi @marcin-de, we have supported processing compressed PF and ACF. This is the latest update link of PPP: https://github.com/azure-rtos/netxduo/commit/e67c70ef358e36887bdb2ff7029807eaf234e97f. It would be much appreciated if you could help to test the latest code to check whether the compression issue you reported is fixed.

Below are the steps to test the new feature of PPP:

  1. Replace the nx_ppp.c and nx_ppp.h in your project with the latest files: https://github.com/azure-rtos/netxduo/tree/master/addons/ppp.
  2. Enable processing compressed PF and ACF by adding the macro 'NX_PPP_COMPRESSION_ENABLE' to nx_user.h or to the project configuration.
  3. Rebuild and run the project.

Thanks a lot. Looking forward to your feedback!