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
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

_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)?

Why do we need two versions in the first place?

Why do we need two versions in the first place?

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> {

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?
Poster
Owner

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.

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.

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``

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).

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.
Poster
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)?

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
Poster
Owner
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
There is no content yet.