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

feature: expose C api cs_pre_trans_delay in SpiDriver #266

Merged
31 changes: 29 additions & 2 deletions src/spi.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,11 @@ pub mod config {
pub duplex: Duplex,
pub bit_order: BitOrder,
pub cs_active_high: bool,
/// On Half-Duplex transactions: `cs_pre_delay_us % 16` corresponds to the number of SPI bit-cycles cs should be activated before the transmission.
/// On Full-Duplex transactions: `cs_pre_delay_us != 0` will add 1 microsecond of cs activation before transmission
pub cs_pre_delay_us: Option<u16>, // u16 as per the C struct has a uint16_t, cf: esp-idf/components/driver/spi/include/driver/spi_master.h spi_device_interface_config_t
///< Amount of SPI bit-cycles the cs should stay active after the transmission (0-16)
pub cs_post_delay_us: Option<u8>, // u8 as per the C struct had a uint8_t, cf: esp-idf/components/driver/spi/include/driver/spi_master.h spi_device_interface_config_t
pub input_delay_ns: i32,
pub polling: bool,
pub allow_pre_post_delays: bool,
Expand Down Expand Up @@ -290,6 +295,22 @@ pub mod config {
self
}

/// On Half-Duplex transactions: `cs_pre_delay_us % 16` corresponds to the number of SPI bit-cycles cs should be activated before the transmission
/// On Full-Duplex transactions: `cs_pre_delay_us != 0` will add 1 microsecond of cs activation before transmission
#[must_use]
pub fn cs_pre_delay_us(mut self, delay_us: u16) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it behave when not in half-duplex mode? Do we get a runtime EspError or does it just get eaten silently? If you are not sure could you test out the cases and report how it acts running?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a very funny thing in fact. I only use the FullDuplex mode.
I am using the AI-Thinker NodeMcu ESP32-S.
And it does make a difference (small one, but there is). However it only seems to take in account the fact that its different from 0.
When I do not set it or put 0, we have no time gap between CS pull-down and the first clock (and you can see the turquoise MISO channel doesn't change its state):

  • 0 cs_pre_delay_us_0

When I set it to 1/16/1000, I get the same small time gap between the CS pull-down and the first clock (and MISO answers):

  • 1 cs_pre_delay_us_1
  • 16 cs_pre_delay_us_16
  • 1000 cs_pre_delay_us_1000

This is all using FullDuplex.

They say in the C API driver include that its not supported, but somewhere else if we look closely we see:
image

In full-duplex mode, only support cs pretrans delay = 1 and without address_bits and command_bits"

So, at least from what I see its that, even though some part of the C doc seems to say that in FullDuplex mode it doesn't work. In reality its more like a on-off switch for a 1us pre-delay in FullDuplex mode, but it does make a big difference for some SPI chips.

Also, as you can see, setting a value higher than 16 (here 1000), doesn't change the behavior compared to 16.

(And sorry for the bad pictures instead of the oscilloscope screen capture, I was lazy to remember the functionality, but I think its still is enough to demonstrate)

Copy link
Contributor Author

@vpochapuis vpochapuis Jun 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further reading of the doc says:

cs_ena_pretrans is not compatible with the Command and Address phases of full-duplex transactions.

But on the field itself:

Amount of SPI bit-cycles the cs should be activated before the transmission (0-16). This only works on half-duplex transactions.

Seems like its not as clear as it could be in the doc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing it out!
We are not using the Command & Address part currently in the driver. So that bit doesn't matter.
Did you also test the post delay? Docs don't indicate that this isn't only working in Half-Duplex.

What version of esp-idf are you using? We could annotate your finding's that a delay is happening in Full-Duplex at least in your tested esp-idf version, but with a warning that it might not be a "stable" option to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This week will be busy at work for reports etc... I will see if I have time, if not it will be next week!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its your PR -> your pacing 😄
i have no stress here

self.cs_pre_delay_us = Some(delay_us);
self
}

/// Add an aditional Amount of SPI bit-cycles the cs should be activated after the transmission (0-16).
/// This only works on half-duplex transactions.
#[must_use]
pub fn cs_post_delay_us(mut self, delay_us: u8) -> Self {
self.cs_post_delay_us = Some(delay_us);
self
}

#[must_use]
pub fn input_delay_ns(mut self, input_delay_ns: i32) -> Self {
self.input_delay_ns = input_delay_ns;
Expand Down Expand Up @@ -324,6 +345,8 @@ pub mod config {
cs_active_high: false,
duplex: Duplex::Full,
bit_order: BitOrder::MsbFirst,
cs_pre_delay_us: None,
cs_post_delay_us: None,
input_delay_ns: 0,
polling: true,
allow_pre_post_delays: false,
Expand Down Expand Up @@ -490,6 +513,8 @@ where
0_u32
} | config.duplex.as_flags()
| config.bit_order.as_flags(),
cs_ena_pretrans: config.cs_pre_delay_us.unwrap_or(0),
cs_ena_posttrans: config.cs_post_delay_us.unwrap_or(0),
post_cb: Some(spi_notify),
..Default::default()
};
Expand Down Expand Up @@ -795,6 +820,8 @@ where
} | config.duplex.as_flags()
| config.bit_order.as_flags(),
post_cb: Some(spi_notify),
cs_ena_pretrans: config.cs_pre_delay_us.unwrap_or(0),
cs_ena_posttrans: config.cs_post_delay_us.unwrap_or(0),
..Default::default()
};

Expand Down Expand Up @@ -1343,8 +1370,8 @@ where
})
}

/// Add an aditional delay of x in uSeconds before transaction
/// between chip select and first clk out
/// Add an aditional Amount of SPI bit-cycles the cs should be activated before the transmission (0-16).
/// This only works on half-duplex transactions.
pub fn cs_pre_delay_us(&mut self, delay_us: u32) -> &mut Self {
self.pre_delay_us = Some(delay_us);

Expand Down
Loading