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

pkg: cncbor: misc fixes #8791

Merged
merged 3 commits into from Apr 4, 2018
Merged

pkg: cncbor: misc fixes #8791

merged 3 commits into from Apr 4, 2018

Conversation

kaspar030
Copy link
Contributor

@kaspar030 kaspar030 commented Mar 16, 2018

Contribution description

Testing in #8467 was limited to native. This PR fixes some problems on ARM:

  1. cn-cbor uses unaligned ntoh[sl](), causing hard faults on ARM. -> added an alignment safe implementation

  2. the unittests parse_hex() had faulty malloc+sscanf. replaced with fmt.

Issues/PRs references

#8467. @

@kaspar030 kaspar030 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Mar 16, 2018
@kaspar030 kaspar030 requested a review from kYc0o March 16, 2018 14:40
@kaspar030
Copy link
Contributor Author

@lorenz9314 can you take a look at this?

@nmeum
Copy link
Member

nmeum commented Mar 16, 2018

Did you consider also submitting these patches upstream?

@kaspar030
Copy link
Contributor Author

Did you consider also submitting these patches upstream?

The original code has a comment like "if you cannot do unaligned accesses, fix these:", so I assumed upstream is not interested. Then again, as is, noone ever used that code on ARM before...

@jnohlgard
Copy link
Member

I see that upstream code comment as more of a "feel free to provide a PR if you need alignment", not a wontfix

@kaspar030
Copy link
Contributor Author

I see that upstream code comment as more of a "feel free to provide a PR if you need alignment", not a wontfix

cabo/cn-cbor#47

@lorenz9314
Copy link
Contributor

@kaspar030 Yep, sure. I pretty much took the parse_hex() function from the original cn-cbor tests. This however seems much more elegant (and functioning) anyway.

@kYc0o
Copy link
Contributor

kYc0o commented Mar 23, 2018

Got the same problem while testing cbor related things.

Should we wait for upstream merge?

Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

Tested ACK. However maybe it's good to wait for upstream changes.

@kYc0o
Copy link
Contributor

kYc0o commented Mar 23, 2018

AND, we didn't spot this because Murdock doesn't build unittests separately, and while building the whole unittests the module posix is added for other tests, so it gets magically imported.

I think we should provide a fix for that. I think @kaspar030 was working already on something like that while doing #8801 ?

@kaspar030
Copy link
Contributor Author

I suggest we merge this, upstreaming might take time. We can remove the patch easily later. Also, this PR includes some other fixes to the package.

@kaspar030
Copy link
Contributor Author

  • squashed

@kYc0o
Copy link
Contributor

kYc0o commented Apr 4, 2018

Then go!

@kYc0o kYc0o merged commit baf1b8f into RIOT-OS:master Apr 4, 2018
@kaspar030 kaspar030 deleted the fix_cn-cbor branch May 11, 2018 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants