runtime: use device endian for kernel/rpc #126

Merged
sb10q merged 1 commits from pca006132/artiq-zynq:master into master 2021-01-22 16:33:29 +08:00
Contributor
Together with https://github.com/m-labs/artiq/pull/1588
pca006132 force-pushed master from 10a07b8c12 to 2c03e281a1 2021-01-21 11:13:28 +08:00 Compare
pca006132 force-pushed master from 2c03e281a1 to b834d9aaf3 2021-01-21 12:22:37 +08:00 Compare
Owner

_kernel isn't a good suffix. It's ARTIQ-specific where this part of the code is otherwise generic (and reusable), and it doesn't describe what is going on.

What about _nativeendian, or even just _native (with a comment explaining that this refers to endianness)?

``_kernel`` isn't a good suffix. It's ARTIQ-specific where this part of the code is otherwise generic (and reusable), and it doesn't describe what is going on. What about ``_nativeendian``, or even just ``_native`` (with a comment explaining that this refers to endianness)?
Contributor

Why do we need two versions in the first place?

Why do we need two versions in the first place?
Owner

The analyzer and moninj are using network order.

The analyzer and moninj are using network order.
sb10q reviewed 2021-01-21 20:54:56 +08:00
@ -136,2 +200,4 @@
}
#[inline]
fn write_bytes_kernel(&mut self, value: &[u8]) -> Result<(), Self::WriteError> {
Owner

Hmm, do we need this (and also *_string_native and *_chunk_native) or would it be simpler to keep network order for the length?

Hmm, do we need this (and also ``*_string_native`` and ``*_chunk_native``) or would it be simpler to keep network order for the length?
Author
Contributor

Not entirely sure, I just duplicated them to minimize the changes to existing code.

Not entirely sure, I just duplicated them to minimize the changes to existing code.
Owner

The analyzer and moninj are using network order.

Though I'm not insisting on that idea; we can instead specify that all ARTIQ firmware protocols always use the endianness of the core device, and have the core device announce its endianness when the host connects.

> The analyzer and moninj are using network order. Though I'm not insisting on that idea; we can instead specify that all ARTIQ firmware protocols always use the endianness of the core device, and have the core device announce its endianness when the host connects.
Owner

Or get it from the device database like the compiler does currently. that would break artiq_rtiomon

<s>Or get it from the device database like the compiler does currently.</s> that would break ``artiq_rtiomon``
Contributor

Though I'm not insisting on that idea; we can instead specify that all ARTIQ firmware protocols always use the endianness of the core device, and have the core device announce its endianness when the host connects.

That would seem like the simpler solution to me (and we might not even need the announcing endianness part – it's not like moninj/the analyzer are used in situtions where the endianness info from the device db wouldn't be available).

> Though I'm not insisting on that idea; we can instead specify that all ARTIQ firmware protocols always use the endianness of the core device, and have the core device announce its endianness when the host connects. That would seem like the simpler solution to me (and we might not even need the announcing endianness part – it's not like moninj/the analyzer are used in situtions where the endianness info from the device db wouldn't be available).
Owner

it's not like moninj/the analyzer are used in situtions where the endianness info from the device db wouldn't be available

artiq_rtiomon doesn't use the device db.

> it's not like moninj/the analyzer are used in situtions where the endianness info from the device db wouldn't be available ``artiq_rtiomon`` doesn't use the device db.
Author
Contributor

_kernel isn't a good suffix. It's ARTIQ-specific where this part of the code is otherwise generic (and reusable), and it doesn't describe what is going on.

What about _nativeendian, or even just _native (with a comment explaining that this refers to endianness)?

Will fix. I was thinking about these functions are just limited to kernel usage comm_kernel.

Actually, would it be better to specify a bool flag? I think the rust compiler may be good enough to inline them. Maybe I should run a benchmark.

> ``_kernel`` isn't a good suffix. It's ARTIQ-specific where this part of the code is otherwise generic (and reusable), and it doesn't describe what is going on. > > What about ``_nativeendian``, or even just ``_native`` (with a comment explaining that this refers to endianness)? Will fix. I was thinking about these functions are just limited to kernel usage `comm_kernel`. Actually, would it be better to specify a bool flag? I think the rust compiler may be good enough to inline them. Maybe I should run a benchmark.
pca006132 force-pushed master from b834d9aaf3 to faa335461d 2021-01-22 13:37:01 +08:00 Compare
Author
Contributor
Updated with https://github.com/m-labs/artiq/pull/1591
sb10q merged commit faa335461d into master 2021-01-22 16:33:29 +08:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#126
No description provided.