-
Notifications
You must be signed in to change notification settings - Fork 1k
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
DTR P2P in firmware #1098
DTR P2P in firmware #1098
Conversation
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.
Generally very good! Nice with documentation and example app.
Some comments in the code.
The app should also be added to .github/workflows/CI.yml
to build it in CI
docs/functional-areas/p2p_DTR_api.md
Outdated
uint8_t source_id; | ||
uint8_t target_id; | ||
uint8_t dataSize; | ||
bool allToAllFlag; |
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.
Another option to using the allToAllFlag
would be to reserve target_id == 0xff for this purpose.
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 that on later commit
docs/functional-areas/p2p_DTR_api.md
Outdated
bool DTRgetPacket(DTRpacket* packet, uint32_t timeout); | ||
``` | ||
|
||
The function blocks for the specified time (in milliseconds) until a packet is received.If the user wants to block indefinitely, the timeout parameter must be set to `portMAX_DELAY`. The function returns true if a packet was received and false otherwise. If a packet is received, the packet is filled with the data received. In case it was received, the packet is filled with the data received and the corresponding packet is released from the queue responsible for the reception of the DTR packets. |
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.
There is a bit of a mix of milliseconds and ticks here as portMAX_DELAY
is in ticks. Maybe change the signature of the function to take ticks?
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.
The function was finally changed to accepting ticks
@@ -0,0 +1,27 @@ | |||
#!/usr/bin/env bash |
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.
This is a pretty generic swarm flashing script. Maybe it should be generalized and documented elsewhere?
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.
It was moved to tools/utils
* | ||
* Crazyflie control firmware | ||
* | ||
* Copyright (C) 2019 Bitcraze AB |
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.
Update year
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 in other files as well
static uint8_t my_id; | ||
static DTRtopology topology = NETWORK_TOPOLOGY; | ||
|
||
void loadTXPacketsForTesting(void){ |
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.
Should we include a "Hello world" as payload?
* | ||
*/ | ||
|
||
#ifndef _P2P_INTERFACE_H_ |
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.
You can also use
#pragma once
void DTRsendP2Ppacket(const DTRpacket* packet) ; | ||
|
||
// Puts the DTR packet in the queue for the token ring to pick up. | ||
void DTRp2pIncomingHandler(P2PPacket *p); |
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.
This function should probably return a bool to indicate if the packet was handled or not. This will allow a user to know if any other action is required.
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 changed it into returning true if it was handled from the protocol and false otherwise.
|
||
void DTRstartCommunication(); | ||
|
||
void DTRInterruptHandler(void *param); |
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.
This function should be renamed as it is not running as a hardware interrupt. The name could cause confusion when reading the code.
src/modules/src/p2pDTR/queueing.c
Outdated
bool DTRgetPacketFromQueue(DTRpacket *packet, DTRQueue_Names qName, uint32_t timeout){ | ||
// notice that xQueuePeek is used instead of xQueueReceive, because the packet is not removed from the queue | ||
//TODO: make a separate function for this | ||
bool received_success; |
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.
Set a default value as this is undefined for variables on the stack.
bool received_success = false;
src/modules/src/p2pDTR/queueing.c
Outdated
} | ||
|
||
bool DTRinsertPacketToQueue(DTRpacket *packet, DTRQueue_Names qName) { | ||
bool res = xQueueSend(*getQueueHandler(qName),(void *) packet, 0) == pdTRUE; |
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.
the return value from xQueueSend()
is technically BaseType_t
and not bool. It is possibly clearer to do
if (pdTRUE != res) ...
, not sure
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.
It was already like this but I will add ( ... )
to make it more visible
I think this looks really good! If you feel it is ready, please set it as non-draft and then I can merge it. |
In this PR a new protocol based on the work of Christos Zosimidis and Klaus Kefferpütz's Lab from Hochschule Augsburg. It gives users the choice to send and receive peer-to-peer in a more robust way since they don't have to worry about missing or lost packets by implementing it as an extra layer on top of the previously implemented P2P API. The sending and receiving of the data are done asynchronously by using 2 different queues. You can find complete documentation of it in the functional-areas folder with the name
p2p_DTR_api.md
and an app layer example is introduced in theapp_p2p_DTR
folder of the examples.