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

Ensure header CRC is written as unsigned int #9783

Merged
merged 1 commit into from Feb 21, 2019

Conversation

Projects
None yet
5 participants
@bridadan
Copy link
Contributor

commented Feb 20, 2019

Description

Fixes #9750. The bootloader merge would fail since the CRC produces an unsigned 32 bit integer, however we specified it as a signed 32 bit integer.

I've tested this on a K64F and ran an update over Pelion, seems to work ok!

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@theotherjimmy - tools team review

Release Notes

@cmonr cmonr requested a review from theotherjimmy Feb 20, 2019

@cmonr cmonr added the needs: review label Feb 20, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2019

@bridadan We've done this fix before (#8788), only to have it reverted (#8915)

Thoughts?

@bridadan bridadan force-pushed the bridadan:fix_unsigned_crc_header branch to 434d86b Feb 21, 2019

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Hey @cmonr, thanks for linking that, I thought this changeset looked familiar 😆

This is the error that appeared in the linked PRs that concerned me: #8788 (comment)

Traceback (most recent call last):
  File "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/make.py", line 71, in wrapped_build_project
    src_dir, build_dir, mcu, *args, **kwargs
  File "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/build_api.py", line 549, in build_project
    merge_region_list(region_list, res, notify)
  File "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/build_api.py", line 422, in merge_region_list
    _fill_header(region_list, region).tofile(header_filename, format='hex')
  File "/builds/ws/mbed-os-ci_cloud-client-test/mbed-cloud-client-example/mbed-os/tools/build_api.py", line 392, in _fill_header
    header.puts(start, struct.pack(fmt, zlib.crc32(ih.tobinarray())))
error: integer out of range for 'L' format code

I've just pushed a change that I believe will solve this issue. I've followed the note in the python docs: https://docs.python.org/2.7/library/zlib.html#zlib.crc32

To generate the same numeric value across all Python versions and platforms use crc32(data) & 0xffffffff. If you are only using the checksum in packed binary format this is not necessary as the return value is the correct 32bit binary representation regardless of sign.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@ARMmbed/mbed-os-maintainers @JanneKiiskila #9783 (comment) Thoughts?

@bridadan Yeah, I'm just being cautious. I thought that this would be the right fix as well, and still thing so, but don't want us to get into a bad state again.

@0xc0170
Copy link
Member

left a comment

Lets run CI !

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Set to 5.12 to sit on master and run additional nightly tests

@bridadan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Hey @cmonr, I'm pretty sure the last commit I pushed should avoid the error: integer out of range for 'L' format code problem. I confirmed the cases that cause that error and also confirmed that bitwise "anding" the values with 0xffffffff forces them to the right size:

import struct
>>> fmt = "<L"
>>> val = 0xffffffff + 1
>>> struct.pack(fmt, val)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
struct.error: integer out of range for 'L' format code
>>> struct.pack(fmt, val & 0xffffffff)
'\x00\x00\x00\x00'
>>> val = -1
>>> struct.pack(fmt, val)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
struct.error: integer out of range for 'L' format code
>>> struct.pack(fmt, val & 0xffffffff)
'\xff\xff\xff\xff'
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 21, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr merged commit 091da53 into ARMmbed:master Feb 21, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9235 cycles (+111 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.