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

Accept byte array payload for raw CAN and diagnostic message formats #7

Open
peplin opened this issue Mar 10, 2014 · 1 comment
Open
Assignees

Comments

@peplin
Copy link
Member

peplin commented Mar 10, 2014

The raw CAN and diagnostic message payload formats (as JSON) expect a hex string for the payload. Ideally we could use an actual number here, but the JSON spec doesn't include base 16 (hex). Instead, we encode it as a string.

This is problematic for a few reasons, namely that it's inefficient (string manipulation), it's prone to byte ordering issues (between, the host, the VI, and CAN bus ordering).

Really, we shouldn't be trying to represent the payload as a single number, even a uint64_t, at all. It's not a single number, it's an array of bytes. There's no 'byte order' for this because the leftmost byte is always...the leftmost byte. There's no "most significant" byte. We introduced a lot of unnecessary complexity by using uint64_t everywhere in the VI instead of uint8_t[8]. It seemed pretty slick at the time, but it's not helping. This became a bigger issue when we added diagnostic requests, which could have a payload much larger than 8 bytes that would obviously not fit in a single uint64_t.

I think what we should do it change the JSON format to accept a JSON array of numbers (i.e. bytes) for each payload. For backwards compatibility, we may choose to also support hex strings for CAN messages and diagnostic request payloads less than or equal to 6 bytes.

@peplin peplin self-assigned this Aug 5, 2014
@peplin peplin added the ready label Sep 10, 2014
@peplin peplin added in progress and removed ready labels Oct 7, 2014
@peplin
Copy link
Member Author

peplin commented Oct 7, 2014

I just pushed an update to the docs in a branch with this change, but I want to update the vi-firmware and openxc-python projects to make sure it isn't too awkward before I commit to it.

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

No branches or pull requests

1 participant