-
Notifications
You must be signed in to change notification settings - Fork 148
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
Conversation
bef9c99
to
376b7a9
Compare
There was a problem hiding this 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.
components/src/radio/hm11/hm11.ads
Outdated
-- As_Stream starts reading without blocking the caller adr read data | ||
-- into Received in a cycle | ||
|
||
type Readed_Position_Procedure is access |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
components/src/radio/hm11/hm11.ads
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
components/src/radio/hm11/hm11.ads
Outdated
Status : out UART_Status); | ||
-- Only Central role is used. | ||
-- Default: False | ||
-- Note: In central role, when power on, module will check if there have |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
examples/shared/hm11/src/drivers.ads
Outdated
As_Stream : Boolean := False); | ||
-- Reads UART5 RX with DMA | ||
|
||
procedure Readed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
No description provided.