Skip to content

Add support for nRF24L01+ radio module #441

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

Merged
merged 1 commit into from
Feb 19, 2025
Merged

Add support for nRF24L01+ radio module #441

merged 1 commit into from
Feb 19, 2025

Conversation

ogorodnik
Copy link
Contributor

No description provided.

@ogorodnik ogorodnik requested a review from pat-rogers February 16, 2025 15:17
@ogorodnik ogorodnik self-assigned this Feb 16, 2025
@CLAassistant
Copy link

CLAassistant commented Feb 16, 2025

CLA assistant check
All committers have signed the CLA.

@ogorodnik ogorodnik force-pushed the nrf24l01p branch 3 times, most recently from 4a87d11 to 8678cf9 Compare February 16, 2025 16:17
pat-rogers
pat-rogers previously approved these changes Feb 16, 2025
Copy link
Contributor

@pat-rogers pat-rogers left a comment

Choose a reason for hiding this comment

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

I recommend changing each occurrence of "Self" to "This" for the sake of consistency with the rest of the library but that's not the end of the world. ;-)

@ogorodnik
Copy link
Contributor Author

Indeed. Self to This renaming done :)

Copy link
Member

@reznikmm reznikmm left a comment

Choose a reason for hiding this comment

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

Will it work for NRF24L01 (without +)?

This is a simple test/example for nRF24L01+ with
STM32F429Disco. Two nRF24L01+ should be connected
to the following board's pins:
TX:
Copy link
Member

Choose a reason for hiding this comment

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

This Markdown file is badly formatted. I suggest list items (or table) to make it readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For NRF24L01 Air_Data_Rate should be Rate_1Mbps.

Copy link
Member

@reznikmm reznikmm left a comment

Choose a reason for hiding this comment

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

I don't like "inversed logic" like No_ACK : Boolean or

  function Is_No_ACK_Allowed
     (This   : NRF24L01P_Driver;
      No_ACK : Boolean) return Boolean;

I prefer ACK : Boolean and Is_ACK_Required. It's simpler to reason in "positive logic" for me.

But I don't object if you merge it as is.

@ogorodnik ogorodnik merged commit 69b17f6 into master Feb 19, 2025
3 of 5 checks passed
@ogorodnik ogorodnik deleted the nrf24l01p branch June 14, 2025 19:27
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.

4 participants