Skip to content

HM-11/cc2541 bluetooth module #444

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
May 6, 2025
Merged

HM-11/cc2541 bluetooth module #444

merged 1 commit into from
May 6, 2025

Conversation

ogorodnik
Copy link
Contributor

No description provided.

@ogorodnik ogorodnik self-assigned this May 4, 2025
@ogorodnik ogorodnik requested a review from pat-rogers May 4, 2025 19:10
@ogorodnik ogorodnik force-pushed the HM_11 branch 2 times, most recently from bef9c99 to 376b7a9 Compare May 4, 2025 21:46
@ogorodnik ogorodnik marked this pull request as draft May 5, 2025 10:09
@ogorodnik ogorodnik marked this pull request as ready for review May 5, 2025 11:36
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.

Nice addition to the library!
I just have a few comments.

-- As_Stream starts reading without blocking the caller adr read data
-- into Received in a cycle

type Readed_Position_Procedure is access
Copy link
Contributor

Choose a reason for hiding this comment

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

English is weird. The past tense of "read" is also spelled "read" (although it is pronounced differently, like the color red). So maybe change the name to "Last_Read_Position_Procedure" or something like that?
(That said, you might want to use "Handler" (or something like that) instead of "Procedure" in both this type name and the one on line 41 too. That's just a matter of taste, but for example I think the name "Discovered_Callback" below is a good name, ie without "_Procedure" in it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for suggestions. Indeed I made a mistake with read. Corrected to: Receive_Handler & Last_Read_Position_Handler

type Readed_Position_Procedure is access
procedure (Closed : out Boolean;
Zero : out Positive);
-- The last readed byte is Zero-1. Closed = True if stream is closed
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Status : out UART_Status);
-- Only Central role is used.
-- Default: False
-- Note: In central role, when power on, module will check if there have
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "there have" should be "there is"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected. In genegal it was so in the device specification.

As_Stream : Boolean := False);
-- Reads UART5 RX with DMA

procedure Readed
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

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.

Very nice!

@ogorodnik ogorodnik merged commit f6a4b8a into master May 6, 2025
3 of 5 checks passed
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.

2 participants