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

Implemented FRSKY X EU LBT #7339

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
7 participants
@phobos-
Copy link
Contributor

phobos- commented Jan 5, 2019

Test flown it and seems to work fine. Binding/telemetry works the same way as frsky x.

@@ -343,15 +345,11 @@ rx_spi_received_e frSkyXHandlePacket(uint8_t * const packet, uint8_t * const pro
case STATE_DATA:
if (cc2500getGdo() && (frameReceived == false)){
uint8_t ccLen = cc2500ReadReg(CC2500_3B_RXBYTES | CC2500_READ_BURST) & 0x7F;
ccLen = cc2500ReadReg(CC2500_3B_RXBYTES | CC2500_READ_BURST) & 0x7F; // read 2 times to avoid reading errors

This comment has been minimized.

@ledvinap

ledvinap Jan 5, 2019

Contributor

The double read was there for some reason ... are you sure it can be removed on all supported HW?

This comment has been minimized.

@phobos-

phobos- Jan 5, 2019

Author Contributor

Yes.
It was there, because of how cc2500 handles GDO0 interrupt - it is configured so that the pin will go high once a certain treshold is reached in the rx fifo. Sometimes readings were incorrect, so someone just put a second read there as a safety measure (maybe sometimes it triggered earlier than it should and the fifo did not contain the final byte yet). In my refactored code it will read it continuously until it reaches the correct amount of data in the fifo - 32 bytes for frsky x and 35 for frsky x eu lbt.

if (ccLen >= packetLength) {
cc2500ReadFifo(packet, packetLength);
uint16_t lcrc= calculateCrc(&packet[3], (packetLength - 7));
if((lcrc >> 8) == packet[packetLength - 4] && (lcrc&0x00FF) == packet[packetLength - 3]){ // check calculateCrc

This comment has been minimized.

@ledvinap

ledvinap Jan 5, 2019

Contributor

You can simply calculate CRC including received CRC bytes. This will produce constant value (IMO zero in FRSKY case) when received CRC is OK, making the check simpler.
(But this won't work if CRC is sent in wrong endianness)

)

This comment has been minimized.

@phobos-

phobos- Jan 6, 2019

Author Contributor

This doesnt work due to endianness as you pointed out. I left it as is, but moved to isValidPacket function to remove nesting.

cc2500ReadFifo(packet, packetLength);
uint16_t lcrc= calculateCrc(&packet[3], (packetLength - 7));
if((lcrc >> 8) == packet[packetLength - 4] && (lcrc&0x00FF) == packet[packetLength - 3]){ // check calculateCrc
if (packet[0] == packetLength - 3) {

This comment has been minimized.

@ledvinap

ledvinap Jan 5, 2019

Contributor

(this can be merged into previous if, decreasing nesting)

This comment has been minimized.

@phobos-

phobos- Jan 6, 2019

Author Contributor

Refactored this logic to a separate function to reduce nesting.

@@ -529,8 +526,20 @@ rx_spi_received_e frSkyXHandlePacket(uint8_t * const packet, uint8_t * const pro
return ret;
}

void frSkyXInit(void)
void frSkyXInit(const rx_spi_protocol_e *spiProtocol)

This comment has been minimized.

@ledvinap

ledvinap Jan 5, 2019

Contributor

spiProtocol can be passed by value

This comment has been minimized.

@phobos-

phobos- Jan 6, 2019

Author Contributor

Agreed, done.

@alexbirkett

This comment has been minimized.

Copy link

alexbirkett commented Jan 5, 2019

This worked for me with Crazybee F3 Pro (on a mobula7) and a Taranis Q X7 transmitter running XJT_EU170317.frk

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 6, 2019

@alexbirkett thanks for testing, any range issues?

@phobos- phobos- force-pushed the phobos-:frsky-x-lbt branch from 5b9235a to dfcbd4c Jan 6, 2019

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 6, 2019

Hmm, diff is now all garbled up, I just removed some whitespaces in most places, thanks to @ledvinap review.

@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Jan 6, 2019

BTW: There is no LBT on telemetry side in FrSkyX protocol? Or is this only partial (but compatible) implementation, ignoring LBT requirement?

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 7, 2019

Hm, not sure how to answer this as frsky code is closed. All I can say is that telemetry works in this implementation. I based the changes on deviationtx and multiprotocol code, and both didnt have any changes to telemetry packet/logic in eu variant.

@MJ666

This comment has been minimized.

Copy link
Contributor

MJ666 commented Jan 7, 2019

I would not expect any handling on the RX side since the TX need to handle this at the first place before even agree on freqencies with the TX?

@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Jan 7, 2019

@MJ666 : RX is transmitting telemetry, so it should use LBT too. Delay of telemetry packet (with respect to non-EU version) is probably added so that RX is able to check if channel is free.

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 7, 2019

Lbt enforced different frequency tune and data rate settings. These cc2500 settings affect both tx and rx, so I guess telemetry packet also complies with lbt. I think you're both correct.

@mikeller
Copy link
Member

mikeller left a comment

Minor fix, the rest looks good.

bool isValidPacket(const uint8_t *packet)
{
uint16_t lcrc= calculateCrc(&packet[3], (packetLength - 7));
if((lcrc >> 8) == packet[packetLength - 4] && (lcrc & 0x00FF) == packet[packetLength - 3] &&

This comment has been minimized.

@mikeller

mikeller Jan 7, 2019

Member

Space after if missing.

@mikeller mikeller added this to the 3.5.5 milestone Jan 7, 2019

@stawiski
Copy link
Contributor

stawiski left a comment

Minor formatting suggestions.

@@ -290,6 +292,19 @@ void frSkyXSetRcData(uint16_t *rcData, const uint8_t *packet)
}
}

bool isValidPacket(const uint8_t *packet)
{
uint16_t lcrc= calculateCrc(&packet[3], (packetLength - 7));

This comment has been minimized.

@stawiski

stawiski Jan 7, 2019

Contributor

Space after lcrc for consistency.

}

if (receiveTelemetryRetryCount >= 5) {
remoteProcessedId = TELEMETRY_SEQUENCE_LENGTH - 1;

This comment has been minimized.

@stawiski

stawiski Jan 7, 2019

Contributor

Double space after equality sign.

@@ -449,7 +453,7 @@ rx_spi_received_e frSkyXHandlePacket(uint8_t * const packet, uint8_t * const pro
break;
#ifdef USE_RX_FRSKY_SPI_TELEMETRY
case STATE_TELEMETRY:
if(cmpTimeUs(micros(), packetTimerUs) >= receiveDelayUs + 400) { // if received or not received in this time sent telemetry data
if(cmpTimeUs(micros(), packetTimerUs) >= receiveDelayUs + telemetryDelayUs) { // if received or not received in this time sent telemetry data

This comment has been minimized.

@stawiski

stawiski Jan 7, 2019

Contributor

Space after if.

packetTimerUs = micros();
frameReceived = true; // no need to process frame again.
remoteToAckId = (remoteAckId + 1) % TELEMETRY_SEQUENCE_LENGTH;
uint8_t remoteNextAckId;

This comment has been minimized.

@stawiski

stawiski Jan 7, 2019

Contributor

If the while loop below does not execute, you would be using uninitialised remoteNextAckId in remoteAckId = remoteNextAckId.

This comment has been minimized.

@phobos-

phobos- Jan 7, 2019

Author Contributor

I'm not sure how to fix this as it is not my code, I only changed indentation here. I'll take a look as it might be a possible bug. Good catch!

This comment has been minimized.

@mikeller

mikeller Jan 7, 2019

Member

Looks like uint8_t remoteNextAckId = remoteToAckId here will fix the bug.

This comment has been minimized.

@phobos-

phobos- Jan 7, 2019

Author Contributor

OK, bear with me, I'll have to test this before pushing.

@phobos- phobos- force-pushed the phobos-:frsky-x-lbt branch from dfcbd4c to b02fb08 Jan 7, 2019

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 7, 2019

@mikeller @stawiski thanks for spotting, hopefully all whitespace issues should be now fixed.

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Jan 7, 2019

I tried to flight test this with a CRAZYBEEF3FR and FrSky X (FCC). Unfortunately I found that with this pull request, and with latest master, the motors do not work on the CRAZYBEEF3FR. Will have to bisect tomorrow.

@phobos- phobos- force-pushed the phobos-:frsky-x-lbt branch from b02fb08 to dfe1e8d Jan 7, 2019

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Jan 7, 2019

Flight tested again and it works. Motors were spinning normally with dshot. My branch is 13 commits behind master.

I think this change is good to go once master is fixed.

@githubDLG

This comment has been minimized.

Copy link
Contributor

githubDLG commented Jan 9, 2019

have download code from phobos's fork:

https://github.com/phobos-/betaflight/tree/frsky-x-lbt?files=1

and complied, flash the hex into crazybeef3fr, then bind with eu-lbt tx and flys well.
FYI....

@mikeller
Copy link
Member

mikeller left a comment

Tested on CRAZYBEEF3FR with FrSky X (non-LBT), this mode works fine. Good work!

@mikeller mikeller merged commit ec083df into betaflight:master Jan 9, 2019

@phobos- phobos- deleted the phobos-:frsky-x-lbt branch Jan 9, 2019

@mikeller mikeller modified the milestones: 3.5.5, 4.0 Feb 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.