[WIP] Expose escape hatch for manual PDI configuration, misc other changes.#243
[WIP] Expose escape hatch for manual PDI configuration, misc other changes.#243david-boles wants to merge 1 commit intoethercrab-rs:masterfrom
Conversation
david-boles
changed the title
| #[cfg_attr(feature = "defmt", derive(defmt::Format))] | ||
| #[repr(u8)] | ||
| pub enum FmmuUsage { | ||
| /// TODO |
There was a problem hiding this comment.
I'll obviously document these properly if you agree with exposing these types publicly.
| type Error; | ||
|
|
||
| /// TODO | ||
| async fn configure<'a, S>( |
There was a problem hiding this comment.
Originally I would have had this just be an FnMut callback instead of its own trait but that's not possible at the moment, as far as I can figure out:
rust-lang/rust#97362 (comment)
There was a problem hiding this comment.
Hey James, just FYI that I should be able to improve the API introduced in this PR with Rust 2024's AsyncFnMut. If you ever want to both uprev the edition and merge this, let me know!
There was a problem hiding this comment.
Yeah, I've been wondering what hotplug groups would look like with AsyncFnMut, as well as general group init. Some interesting experiment opportunities to look at when I have some time!
I'm sorry to leave you hanging out to dry on this PR, and I have more bad news unfortunately; I'm swamped with FT work and other projects at the moment, so I don't see myself revisiting this PR any time soon. My kneejerk reaction is that it exposes too much of EtherCrab's internals verbatim and feels like a bandaid over either a broken SubDevice, or a bug in EtherCrab, but I'd need to get back up to speed with the original issue and this PR before making more informed comments.
In the meantime, I'd encourage you to try the latest master commits to see if anything's improved, and continue maintaining a fork of EtherCrab with the changes you need to get your SubDevice working.
| /// Configure PDI using the PDO assignments in CoE registers. | ||
| #[derive(Clone, Copy)] | ||
| pub struct ConfigureSubdevicePdiBasedOnCoe; | ||
| impl ConfigureSubdevicePdi for ConfigureSubdevicePdiBasedOnCoe { |
There was a problem hiding this comment.
These implementations should be unchanged; I'm just exposing them as separate callbacks. This would have been useful when I was first debugging and wanted to try EEPROM-based setup even though my device supports CoE.
| @@ -102,12 +459,13 @@ where | |||
| /// Second state configuration (PRE-OP -> SAFE-OP). | |||
| /// | |||
| /// PDOs must be configured in the PRE-OP state. | |||
There was a problem hiding this comment.
- Review the doc comments on modified functions
| /// TODO | ||
| pub fn as_raw_ref_mut(&mut self) -> SubDeviceRef<'a, &mut SubDevice> { | ||
| SubDeviceRef { maindevice: self.maindevice, configured_address: self.configured_address, state: self.state.deref_mut() } | ||
| } |
There was a problem hiding this comment.
This is maybe a very me-specific problem but I wanted to pass the SubDeviceRefs to a trait object function, so I needed a concrete type.
| } | ||
|
|
||
| /// Get the index of this subdevice in the overall EtherCAT network when it was first initialized. | ||
| pub fn topology_address_at_init(&self) -> u16 { |
There was a problem hiding this comment.
Rather than rely on ethercrab continuing to configure addresses in the future based on a fixed base plus the position in the topology, I figured I'd add that information to the API explicitly.
| } | ||
|
|
||
| /// TODO | ||
| pub fn pdi_range(&self) -> Range<u32> { |
There was a problem hiding this comment.
In my case I'm actually doing some manual interaction with the PDI, so it was helpful to know the entire range for the group.
|
Hey David, thanks or posting this PR. I'm afraid I don't have time to review it in depth at the moment as I started fulltime work not too long ago and am busy with that. I'd really appreciate a new demo in |
david-boles
force-pushed
the
expose_pdi_escape_hatch
branch
from
eb63770 to
6810e03
Compare
| let sm_config = subdevice | ||
| .write_sm_config(sm_idx, &sync_managers[usize::from(sm_idx)], len_bytes) | ||
| .await | ||
| .expect("configuring sync manager"); | ||
| let desired_sm_type = match direction { | ||
| PdoDirection::MasterRead => SyncManagerType::ProcessDataRead, | ||
| PdoDirection::MasterWrite => SyncManagerType::ProcessDataWrite, | ||
| }; | ||
| subdevice | ||
| .write_fmmu_config(len_bits, fmmu_idx, global_offset, desired_sm_type, &sm_config) | ||
| .await | ||
| .expect("configuring fmmu"); | ||
|
|
||
| Ok(PdiSegment { | ||
| bit_len: usize::from(len_bits), | ||
| bytes: start_offset.up_to(*global_offset), | ||
| }) |
There was a problem hiding this comment.
These are the important lines that this PR facilitates; the ability for a user to configure the SMs and FMMUs of their subdevices themselves.
While avoiding substantially changing the existing EtherCrab PDI setup flow.
2 participants
Hey James; following up on #228. These are the changes I needed to make use of ethercrab with my devices and in the context of the rest of my system. I'm not sure this is all realistically upstreamable, but I'd love to merge however much of it you'd like to take!
ESI files ended up missing a bunch of information that I needed (e.g. decomposing bytes into bit flags, my-system-specific representations and metadata for the ESI variables) so I didn't end up going down that path after all; represented here is a just a ~minimum-viable escape hatch to allow manual SM and FMMU configuration... plus a few other miscellaneous changes that I wanted.