M-Labs
/
artiq-zynq
10
1
Fork 19
Code Issues 13 Pull Requests Releases Wiki Activity

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
Conversation 9 Commits 1 Files Changed 7 +31 -51
pca006132 commented 2021-01-21 11:12:22 +08:00
Contributor
Copy Link

Together with https://github.com/m-labs/artiq/pull/1588

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
sb10q commented 2021-01-21 20:48:21 +08:00
Owner
Copy Link

_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)?
dpn commented 2021-01-21 20:52:09 +08:00
Collaborator
Copy Link

Why do we need two versions in the first place?

Why do we need two versions in the first place?
sb10q commented 2021-01-21 20:53:23 +08:00
Owner
Copy Link

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
src/runtime/src/proto_core_io.rs Outdated
@ -136,2 +200,4 @@
}
#[inline]
fn write_bytes_kernel(&mut self, value: &[u8]) -> Result<(), Self::WriteError> {
sb10q commented 2021-01-21 20:54:56 +08:00
Owner
Copy Link

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?
pca006132 commented 2021-01-22 10:32:41 +08:00
Author
Contributor
Copy Link

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.
sb10q commented 2021-01-21 20:58:52 +08:00
Owner
Copy Link

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.
sb10q commented 2021-01-21 21:00:05 +08:00
Owner
Copy Link

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``
dpn commented 2021-01-21 21:01:21 +08:00
Collaborator
Copy Link

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).
sb10q commented 2021-01-21 21:16:39 +08:00
Owner
Copy Link

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.
pca006132 commented 2021-01-22 10:34:09 +08:00
Author
Contributor
Copy Link

_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
pca006132 commented 2021-01-22 13:53:34 +08:00
Author
Contributor
Copy Link

Updated with https://github.com/m-labs/artiq/pull/1591

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.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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.
Delete Branch "pca006132/artiq-zynq:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?