Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for bit ordering of li, vn, and mode #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ProudGecko
Copy link

The original bit order was wrong for the first byte of the NTP packet. For example, settings ntp[0] = 0x1C, results in li = 0, vn = 7, mode = 0. The correct answers are li = 0, vn = 3, mode = 4.

Here's a screenshot of the visual studio debugger showing the issue.
image

The original bit order was wrong for the first byte of the NTP packet.  For example, settings ntp[0] = 0x1C, results in li = 0, vn = 7, mode = 0.  The correct answers are li = 0, vn = 3, mode = 4.
@lettier
Copy link
Owner

lettier commented Oct 21, 2017

Hello @ProudGecko and thank you for your contribution.

The order of bit fields within a unit is implementation-defined. For your particular implementation, the order appears to be low-order to high-order hence why rearranging the order to mode, vn, and li outputs the correct values on your machine. Since it is implementation-defined, it could be low-order to high-order, vice versa, or something else.

Ultimately the order over the wire is big endian with the li, vn, and mode byte being from high-order to low-order.

If we cared about the individual fields in the li, vn, and mode byte it would be better to use uint8_t li_vn_mode;, use bitwise operators, and remove all of the bit fields.

Since we do not bother with these particular fields individually, we will keep their order as-is in the interest of clarity by following the RFC for the purposes of the article.

👍

High-Order to Low-Order

>>>>>>>>>>>>>>>
00 | 011 |  011
---------------
 0 |   3 |    3
---------------
li |  vn | mode
---------------
Low-Order to High-Order

>>>>>>>>>>>>>>>
11 | 011 |  000
---------------
 3 |   6 |    0
---------------
li |  vn | mode
---------------

or

<<<<<<<<<<<<<<<
 000 | 110 | 11
---------------
   0 |   6 |  3
---------------
mode |  vn | li
---------------

@ProudGecko
Copy link
Author

That's true about implementation specific endianness; however, you've included a C structure definition that assumes a big-endian architecture even though it appears you've built and run this sample on an x86 machine. At the least a comment in the structure would be useful for others that may scratch there head over this.

Thanks for your response!
David

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants