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

Broadcast support #15

Closed
wants to merge 2 commits into from
Closed

Conversation

whoenig
Copy link
Contributor

@whoenig whoenig commented Sep 23, 2016

This adds basic support for receiving broadcasted messages in the nrf firmware.
1.) It fixes the handling of noAck vs. Ack in the message receive handler.
2.) It uses local address 1, to listen to a separate "broadcast" address, which can be shared amongst Crazyflies.
3.) It adds a new syslink type so that the STM32 firmware can distinguish between the two. (The STM32 changes will be part of another pull request though.)

This has been tested as part of the Crazyswarm project.

@ataffanel
Copy link
Member

When trying the change I cannot connect with Crazyradio anymore, is there something changed with the default address?

@ataffanel
Copy link
Member

Thanks! Lets merge that when the NRF part is merged

@whoenig
Copy link
Contributor Author

whoenig commented Sep 26, 2016

I think this change should actually work with "old" and "new" firmware as it only adds features. Am I understanding correctly that you used the STM32 firmware (as in the master branch), this change in the pull request for the NRF firmware and the result was that you couldn't connect using the cfclient?

@ataffanel
Copy link
Member

I tested both with the Master stm and with the pull request on the STM. I am going to test again to make sure.

@ataffanel
Copy link
Member

It actually works with a 99.55 radio from bitcraze/crazyradio-firmware#31 but not with a stock 0.53. This is quite strange I am investigating what is happening.

@ataffanel
Copy link
Member

Sorry for the long delay, things have been busy latelly. So I found why things are not working: The noAck Vs ack is the problem. This bit is actually documented as no_ack by the nRF24LU1:

No Acknowledgment flag (NO_ACK)

The Selective Auto Acknowledgement feature controls the NO_ACK flag.
This flag is only used when the auto acknowledgement feature is used. Setting the flag high, tells the
receiver that the packet is not to be auto acknowledged.

So with this PR crazyflie is not acking anymore to the current stable radio that follows this bit definition. But the last PR for crazyradio sets the bit the oposite way as from the doc. This is a sniff of scanning with the new radio:

Pid 02, Noack 1, -47dBm, ff 
Pid 02, Noack 1, -47dBm, ff 
Pid 02, Noack 1, -34dBm, ff 
Pid 02, Noack 1, -34dBm, ff 

I am looking at the Crazyradio to find what is happening there. Since the current Crazyflie did not care about no_ack this is why this new radio did not break the communication.

@whoenig
Copy link
Contributor Author

whoenig commented Oct 13, 2016

Ahh that makes sense - great investigation! So could we basically flip it in both the radio firmware and CF firmware to fix the issue?

@ataffanel
Copy link
Member

Yes that's the plan. I have not had time to check if inverting the bit in the Crazyradio was possible thought, I really hope it is.

@whoenig
Copy link
Contributor Author

whoenig commented Oct 13, 2016

I could look into that too, once I am back (next Tuesday).

@whoenig
Copy link
Contributor Author

whoenig commented Nov 19, 2016

I looked into that more and I am not sure what's going on. In the released firmware, the NO_ACK command was disabled (i.e. 0x06 for https://github.com/bitcraze/crazyradio-firmware/blob/master/firmware/src/radio.c#L123). If I send data with such a setting, I get exactly what the documentation says (acknowledgement requested => bit = 0). However, once I enable the setting the NO_ACK bit seems to be inverted in both cases (and hence becomes an ack-bit).

So it seems to me there is no way of having a compatible change which uses the ack bit. However, what I can do is changing the logic and decide based on the used pipe whether it is a broadcast or not (similar to this logic here: https://github.com/USC-ACTLab/crazyflie2-nrf-firmware/blob/devBroadcast/src/main.c#L168). This is hacky of course, but otherwise I am afraid we would need to require users to update the radio firmware whenever they want a new a newer nrf-firmware...

What do you think?

@ataffanel
Copy link
Member

This looks like a hardware radio bug which is unfortunate. The broadcast pipe looks similar to what is happening in regular ethernet network: one address is defined as broadcast. It seems like a good solution to me.

Practically do you think we need to define a broadcast address scheme (like all addresses that end with ff are broadcast). If so we need to choose how much of the address is the "network id" and how much is the "copter id/broadcast".

I am not sure how that will be compatible with Crazyflie 1 but I ok for not supporting broadcast in CF1 since I do not think anyone will have use for it.

@@ -202,7 +203,7 @@ void esbInterruptHandler()
}

// If this packet is a retry, send the same ACK again
if (isRetry(pk)) {
if (pk->ack && isRetry(pk)) {
Copy link
Member

Choose a reason for hiding this comment

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

Shall we test for the pipe here instead of testing for the ACK bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that should fix it. Sorry, I didn't manage to actually test that so far, otherwise I would have updated the PR already.

Copy link
Member

Choose a reason for hiding this comment

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

No problem I can update and test it so that we get rid of the PR.

@ataffanel
Copy link
Member

Finally merged this one :-). I think I have done something that upset Github since it did not recognized my merge (I rebased before pushing, that might have been my mistake). Anyway your commits are in @whoenig and I modified them to use the address to mach broadcast packets. Thanks!

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