net/hdlc: initial hdlc layer by maxvankessel · Pull Request #9200 · RIOT-OS/RIOT

maxvankessel · 2018-05-25T12:52:09Z

I've created with help from @jia200x a HDLC layer (netdev layer), it's part of the PPP stack.
It should be layered on top of a PPPoS netdev layer, which handles incoming bytes and the ACCM map.
But it can also be layered on top of a generic modem driver (netdev).

This layers knows it's a netdev_layer.

I would like some feedback on the buffers is the hdlc_t, I don't like it this way.

kaspar030

Nice PR so far, thanks!

kaspar030 · 2018-05-27T13:19:29Z

sys/include/net/hdlc/hdr.h

+ * @brief HDLC types
+ */
+typedef enum {
+    HDLC_TYPE_RECEIVE_READY     = 0,        /**< ready receive type */


I find the shifts quite confusing. Would you mind using hex values?

I agree, this looks a bit weird here

kaspar030 · 2018-05-27T13:23:21Z

sys/include/net/hdlc/hdr.h

+/**
+ * @brief HDLC Supervisory Frames
+ */
+typedef struct __attribute__((packed)) {


Please don't use bitfields for wire formats. The actual order is implementation defined and thus cannot be relied upon.

smlng

some coding style issue, mostly missing spaces before parentheses, please adapt.

sys/include/net/hdlc.h

sys/include/net/hdlc/fcs.h

sys/include/net/hdlc/hdr.h

sys/include/net/hdlc/fcs.h

sys/include/net/hdlc.h

sys/include/net/hdlc/fcs.h

smlng · 2018-05-28T09:02:34Z

sys/include/net/hdlc/hdr.h

+ * @brief HDLC types
+ */
+typedef enum {
+    HDLC_TYPE_RECEIVE_READY     = 0,        /**< ready receive type */


I agree, this looks a bit weird here

maxvankessel · 2018-05-30T17:58:06Z

I've updated the source accordingly.

jia200x · 2019-01-03T13:06:22Z

ping @maxvankessel

jia200x · 2019-01-03T13:15:58Z

I like the approach of keeping HDLC and PPPoS in different layers as you did! I can ACK the fundamentals

jia200x · 2019-01-03T13:21:19Z

Some feedback from Vera++:

sys/net/link_layer/hdlc/hdlc.c:284: too many consecutive empty lines
sys/include/net/hdlc.h:65: comma should be followed by whitespace
sys/include/net/hdlc/fcs.h:72: line is longer than 100 characters

miri64 · 2019-01-03T13:33:43Z

sys/include/net/hdlc/hdr.h

+ *  +----------+----------+----------+
+ *  |   Flag   | Address  | Control  |
+ *  | 01111110 | 11111111 | 00000011 |
+ *  +----------+----------+----------+


I'm unsure how this translates to the header defined below.

The flag is not a part of the header but a delimiter of the packet

miri64 · 2019-01-03T13:34:39Z

sys/include/net/hdlc/hdr.h

+ *  +----------+----------+----------+
+ *  |   Flag   | Address  | Control  |
+ *  | 01111110 | 11111111 | 00000011 |
+ *  +----------+----------+----------+


For it to be rendered correctly as pixelart in the rendered doc, this needs to be indented some spaces (not sure how many, please check yourself with make doc)

miri64 · 2019-07-30T15:26:52Z

Ping @maxvankessel

jia200x · 2019-11-01T13:19:08Z

ping @maxvankessel

maxvankessel · 2019-11-02T10:16:57Z

I will have a look at this. Is it still useful to implement this layer after all this time?

miri64 · 2019-11-02T10:30:27Z

I will have a look at this. Is it still useful to implement this layer after all this time?

We still want PPP support, so yes :-).

maxvankessel · 2019-11-02T10:44:13Z

Okay, give me some time to get back into it, it's been a while

comments address, wait for author

benpicco · 2020-05-10T17:02:07Z

sys/include/net/hdlc/fcs.h

+static uint16_t fcstab[256] = {
+    0x0000, 0x1189, 0x2312, 0x329b, 0x4624, 0x57ad, 0x6536, 0x74bf,
+    0x8c48, 0x9dc1, 0xaf5a, 0xbed3, 0xca6c, 0xdbe5, 0xe97e, 0xf8f7,
+    0x1081, 0x0108, 0x3393, 0x221a, 0x56a5, 0x472c, 0x75b7, 0x643e,
+    0x9cc9, 0x8d40, 0xbfdb, 0xae52, 0xdaed, 0xcb64, 0xf9ff, 0xe876,
+    0x2102, 0x308b, 0x0210, 0x1399, 0x6726, 0x76af, 0x4434, 0x55bd,
+    0xad4a, 0xbcc3, 0x8e58, 0x9fd1, 0xeb6e, 0xfae7, 0xc87c, 0xd9f5,
+    0x3183, 0x200a, 0x1291, 0x0318, 0x77a7, 0x662e, 0x54b5, 0x453c,
+    0xbdcb, 0xac42, 0x9ed9, 0x8f50, 0xfbef, 0xea66, 0xd8fd, 0xc974,
+    0x4204, 0x538d, 0x6116, 0x709f, 0x0420, 0x15a9, 0x2732, 0x36bb,
+    0xce4c, 0xdfc5, 0xed5e, 0xfcd7, 0x8868, 0x99e1, 0xab7a, 0xbaf3,
+    0x5285, 0x430c, 0x7197, 0x601e, 0x14a1, 0x0528, 0x37b3, 0x263a,
+    0xdecd, 0xcf44, 0xfddf, 0xec56, 0x98e9, 0x8960, 0xbbfb, 0xaa72,
+    0x6306, 0x728f, 0x4014, 0x519d, 0x2522, 0x34ab, 0x0630, 0x17b9,
+    0xef4e, 0xfec7, 0xcc5c, 0xddd5, 0xa96a, 0xb8e3, 0x8a78, 0x9bf1,
+    0x7387, 0x620e, 0x5095, 0x411c, 0x35a3, 0x242a, 0x16b1, 0x0738,
+    0xffcf, 0xee46, 0xdcdd, 0xcd54, 0xb9eb, 0xa862, 0x9af9, 0x8b70,
+    0x8408, 0x9581, 0xa71a, 0xb693, 0xc22c, 0xd3a5, 0xe13e, 0xf0b7,
+    0x0840, 0x19c9, 0x2b52, 0x3adb, 0x4e64, 0x5fed, 0x6d76, 0x7cff,
+    0x9489, 0x8500, 0xb79b, 0xa612, 0xd2ad, 0xc324, 0xf1bf, 0xe036,
+    0x18c1, 0x0948, 0x3bd3, 0x2a5a, 0x5ee5, 0x4f6c, 0x7df7, 0x6c7e,
+    0xa50a, 0xb483, 0x8618, 0x9791, 0xe32e, 0xf2a7, 0xc03c, 0xd1b5,
+    0x2942, 0x38cb, 0x0a50, 0x1bd9, 0x6f66, 0x7eef, 0x4c74, 0x5dfd,
+    0xb58b, 0xa402, 0x9699, 0x8710, 0xf3af, 0xe226, 0xd0bd, 0xc134,
+    0x39c3, 0x284a, 0x1ad1, 0x0b58, 0x7fe7, 0x6e6e, 0x5cf5, 0x4d7c,
+    0xc60c, 0xd785, 0xe51e, 0xf497, 0x8028, 0x91a1, 0xa33a, 0xb2b3,
+    0x4a44, 0x5bcd, 0x6956, 0x78df, 0x0c60, 0x1de9, 0x2f72, 0x3efb,
+    0xd68d, 0xc704, 0xf59f, 0xe416, 0x90a9, 0x8120, 0xb3bb, 0xa232,
+    0x5ac5, 0x4b4c, 0x79d7, 0x685e, 0x1ce1, 0x0d68, 0x3ff3, 0x2e7a,
+    0xe70e, 0xf687, 0xc41c, 0xd595, 0xa12a, 0xb0a3, 0x8238, 0x93b1,
+    0x6b46, 0x7acf, 0x4854, 0x59dd, 0x2d62, 0x3ceb, 0x0e70, 0x1ff9,
+    0xf78f, 0xe606, 0xd49d, 0xc514, 0xb1ab, 0xa022, 0x92b9, 0x8330,
+    0x7bc7, 0x6a4e, 0x58d5, 0x495c, 0x3de3, 0x2c6a, 0x1ef1, 0x0f78
+};


There is already sys/checksum/crc16_ccitt.c

stale · 2020-11-12T03:28:49Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

jia200x · 2023-04-18T11:12:43Z

hi @maxvankessel

I would really like to move forward with this. Do you plan to continue working on this anytime?
Otherwise, I can offer to take over this PR (keeping your authorship, of course)

maxvankessel · 2023-04-18T11:39:43Z

Hello @jia200x,
Im not planning to revive this pr. You're free to use it.

net/hdlc: initial hdlc layer

991a6f6

kaspar030 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT labels May 27, 2018

kaspar030 requested changes May 27, 2018

View reviewed changes

smlng requested a review from jia200x May 28, 2018 08:03

smlng previously requested changes May 28, 2018

View reviewed changes

net/hdlc: resolved pr comments

3bdd365

maxvankessel force-pushed the pr/hdlc_layer branch from e1ec8b9 to d097afa Compare May 30, 2018 10:54

net/hdlc: added hdlc layer to makefile dependency

6419d48

maxvankessel force-pushed the pr/hdlc_layer branch from d097afa to 6419d48 Compare May 30, 2018 11:04

maxvankessel mentioned this pull request May 30, 2018

drivers/pppos: initial ppp over serial layer #9246

Closed

maxvankessel mentioned this pull request May 30, 2018

[Link layer] GNRC PPP #5470

Closed

jia200x added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Jan 3, 2019

miri64 requested changes Jan 3, 2019

View reviewed changes

benpicco reviewed May 10, 2020

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Nov 12, 2020

stale bot closed this Dec 13, 2020

Conversation

maxvankessel commented May 25, 2018

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxvankessel commented May 30, 2018

Uh oh!

jia200x commented Jan 3, 2019

Uh oh!

jia200x commented Jan 3, 2019

Uh oh!

jia200x commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jul 30, 2019

Uh oh!

jia200x commented Nov 1, 2019

Uh oh!

maxvankessel commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Nov 2, 2019

Uh oh!

maxvankessel commented Nov 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Nov 12, 2020

Uh oh!

jia200x commented Apr 18, 2023

Uh oh!

maxvankessel commented Apr 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jia200x commented Jan 3, 2019 •

edited

Loading

maxvankessel commented Nov 2, 2019 •

edited

Loading