Skip to content

Conversation

@zqb-all
Copy link
Contributor

@zqb-all zqb-all commented Dec 19, 2019

fix size in Layout of the CRC tag

Copy link
Member

@geky geky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zqb-all, good catch and thanks for the PR! Some comments below.

SPEC.md Outdated
tag data
[-- 32 --][-- 32 --|--- variable length ---]
[1| 3| 8 | 10 | 10 ][-- 32 --|--- (size) ---]
[1| 3| 8 | 10 | 10 ][-- 32 --|--- (size - 4) ---]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this actually be (size - 32) since the other fields are in bits?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this field should in bits, but size is in bytes, maybe (size * 8 - 32) is better ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Good point, you're right that is better.

[1| 3| 8 | 10 | 10 ][-- 32 --|--- (size) ---]
[1| 3| 8 | 10 | 10 ][-- 32 --|--- (size - 4) ---]
^ ^ ^ ^ ^ ^- crc ^- padding
| | | | '- size (12)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this 12 mean here? The size could be any value.

@zqb-all zqb-all force-pushed the patch-1 branch 2 times, most recently from e10636c to 7fec59a Compare December 24, 2019 03:17
@geky
Copy link
Member

geky commented Jan 27, 2020

Hi @zqb-all, sorry about the absence. Does the updated comment make sense to you?

If you're not able to update the PR I can bring it in and make the suggested changes on the next release.

1.fix size in Layout of the CRC tag
2.update (size) to (size * 8)
@zqb-all
Copy link
Contributor Author

zqb-all commented Feb 2, 2020

Hi @geky, thanks for you reply, I am glad to update the PR.

@geky geky merged commit 38024d5 into littlefs-project:master Mar 31, 2020
@geky
Copy link
Member

geky commented Apr 1, 2020

Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants