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

Lora phy spidevice #2110

Closed
wants to merge 1 commit into from
Closed

Conversation

CBJamo
Copy link
Contributor

@CBJamo CBJamo commented Oct 24, 2023

This depends on the breaking change in embassy-rs/lora-phy#35, thus the patch sections and draft status.

I suspect that the generics and error handling can be done better in the new SubGhzSpiDevice, but I want to get something up, as I don't have the hardware to test this.

@CBJamo CBJamo marked this pull request as draft October 24, 2023 13:23
Operation::Transfer(read, write) => self.bus.transfer(read, write).await?,
Operation::TransferInPlace(buf) => self.bus.transfer_in_place(buf).await?,
#[cfg(not(feature = "time"))]
Operation::DelayUs(_) => return Err(SpiDeviceError::DelayUsNotSupported),
Copy link
Member

Choose a reason for hiding this comment

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

try blocks catch ? but not return. so this will return without deasserting NSS.

Copy link
Contributor Author

@CBJamo CBJamo Oct 24, 2023

Choose a reason for hiding this comment

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

I thought that might be an issue, but I stole that chunk directly from the embassy-embedded-hal SpiDevice. So we should fix it there as well.

E: ref https://github.com/embassy-rs/embassy/blob/b3879ec22346ac38a409512e147fe8f7137f858a/embassy-embedded-hal/src/shared_bus/asynch/spi.rs#L77C60-L77C60

Copy link
Member

Choose a reason for hiding this comment

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

oh, oops. yes please! :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a stab at this, it works but I feel like there should be a less verbose/cleaner way of doing it.

@lucasgranberg
Copy link
Contributor

Tested on wio-e5 module (stm32wle). Join sent to network so it should be good to go.

@CBJamo CBJamo force-pushed the lora-phy-spidevice branch from be856a2 to a9c8b0e Compare October 25, 2023 05:31
Operation::Write(buf) => bus.write(buf).await?,
Operation::Transfer(read, write) => bus.transfer(read, write).await?,
Operation::TransferInPlace(buf) => bus.transfer_in_place(buf).await?,
Operation::Read(buf) => bus.read(buf).await.map_err(|e| SpiDeviceError::Spi(e))?,
Copy link
Member

Choose a reason for hiding this comment

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

you can do t.map_err(SpiDeviceError::Spi) which is slightly shorter.

@CBJamo CBJamo force-pushed the lora-phy-spidevice branch 2 times, most recently from 0ed58e5 to 674a373 Compare October 26, 2023 01:17
Copy link
Member

@lulf lulf left a comment

Choose a reason for hiding this comment

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

Nice work, looks good to me. Would it make sense to move it to embassy-stm32 eventually when this and the bits have landed?

@Dirbaio
Copy link
Member

Dirbaio commented Oct 26, 2023

yes! there was some discussion in the lora matrix about this. I think the best would be to split the trait to a separate lora-phy-driver crate so embassy-stm32 can depend just on that, so it needs bumping less often.

And then remove embassy-lora, and perhaps move the lora examples to the lora repo.

stm32wl SpiDevice support

Remove return from try block in shared_bus and stm32wl
@CBJamo CBJamo force-pushed the lora-phy-spidevice branch from 674a373 to 34bbc45 Compare November 1, 2023 10:38
@plaes
Copy link
Contributor

plaes commented Dec 13, 2023

@CBJamo I guess we can close this now as all these resources have been migrated under lora-rs.

@Dirbaio Dirbaio closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants