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

Add SFHSS-SPI-RX #6865

Merged
merged 9 commits into from Oct 7, 2018

Conversation

Projects
None yet
3 participants
@chibaron
Copy link
Contributor

chibaron commented Oct 1, 2018

Add new function S-FHSS SPI RX. please help to review.
This is works on the FC board on which CC2500-SPI (FC where FrSky-SPI-RX is already running).

  1. Add cli setting value rx_spi_protocol parameter 'SFHSS'. This is change protocol.
  2. Add cli command 'rx_bind'. This is the same function as frsky_bind. (Should I replace frsky_bind?)
  3. Add cli setting value 'sfhss_spi_tx_id' and 'sfhss_spi_offset'. This is sfhss-bind parameter.
  4. Add cc2500_common.c. Common processing with Frsky. For reduce code size, but flash overflow at CrazyBeeF3FR.

Detail

Stack

+--------------------------+
|            RX            |
|--------------------------+
|           RX_SPI         |
+--------------------------+
|       frsky_shared       | 
+---------+---------+      |
| frsky_d | frsky_x |      |
+=========+=========+======+
|     rx_cc2500 (driver)   |
+--------------------------+

  |  add S-FHSS
  v

+---------------------------------------------+
|                      RX                     |
|---------------------------------------------+
|                    RX_SPI                   |
+---------------------------------+-----------+
|       frsky_shared              |   sfhss   |
+---------+---------+    +---------------+    |
| frsky_d | frsky_x |    | cc2500_common |    |
+=========+=========+====+===============+====+
|               rx_cc2500 (driver)            |
+---------------------------------------------+

cc2500_common.c function.

  • Bind control (switch and cli)
  • RX-LED (add blink function)
  • CC2500-GDO-Pin control
  • PA-LNA-Pin control
  • Diversity-Pin control

Development FC

The FC used in this development is as follows

FC Target comment
CrazyBeeF3FR CrazyBeeF3FR code size over!
Betafpv F4 Brushed (Frsky-OSD) MATEKF411RX -
Betafpv F4 Brushless (Frsky-OSD) MATEKF411RX -
@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 1, 2018

Tested with MATEKF411RX using Futaba T14SG in S-FHSS mode. It works, so the basic function is okay.

@jflyper
Copy link
Contributor

jflyper left a comment

There are numerous coding style violations and inconsistencies. Commented are just examples. Please fix them. For style standard, refer to
https://github.com/betaflight/betaflight/blob/master/docs/development/CodingStyle.md

Show resolved Hide resolved src/main/rx/cc2500_sfhss.c Outdated
void cc2500LedOff(void);
void cc2500LedBlink(timeMs_t blinkms);
bool cc2500SpiInit(void);

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

Trailing new line.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

if (sfhssRecv(packet)){
if (sfhssPacketParse(packet, true)){
missingPackets = 0;
if( GET_COMMAND(packet) & 0x8 ){

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

Trailing space at the end of the line & 0x8 ){.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

}
}
cc2500Strobe(CC2500_SRX);
}else if (cmpTimeUs(currentPacketReceivedTime, nextFrameReceiveStartTime) > 0) {

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

} else if (

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

if (cc2500checkBindRequested(true)) {
cc2500LedOn();
initTuneRx();
SET_STATE( STATE_BIND_TUNING1 );

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

SET_STATE(STATE_BIND_TUNING1);
(Consistent with others.)

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

if (sfhss_channel > 29) {
sfhss_channel -= 31;
}
}while( sfhss_channel < 0);

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

} while (

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

cc2500WriteReg(CC2500_3E_PATABLE, 0xFF);

for(unsigned c = 0;c < 30; c++)
{ //calibrate all channels

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

Comment line by itself.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

cc2500WriteReg(CC2500_2E_TEST0, 0x0B);
cc2500WriteReg(CC2500_3E_PATABLE, 0xFF);

for(unsigned c = 0;c < 30; c++)

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

for ( ... ) {

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

if( GET_COMMAND(packet) & 0x8 ){
nextFrameReceiveStartTime = currentPacketReceivedTime + NEXT_CH_TIME_SYNC2;
frame_recvd |= 0x2; /* ch5-8 */
}else{

This comment has been minimized.

@jflyper

jflyper Oct 1, 2018

Contributor

} else {

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 1, 2018

  1. Add cli command 'rx_bind'. This is the same function as frsky_bind. (Should I replace frsky_bind?)
    I prefer to use consolidated rx_bind, but rx_bind may be too broad... Sounds like it works for all receivers.
  1. Add cli setting value 'sfhss_spi_tx_id' and 'sfhss_spi_offset'. This is sfhss-bind parameter.
    Do we need _spi_ here?
    If this is to emphasize onboard receiver function, why not use rx_spi_bind (still too broad).

sfhss_spi_bind, sfhss_spi_tx_id and sfhss_spi_offset
or
sfhss_bind, sfhss_tx_id and sfhss_offset?

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 1, 2018

Also, please note that there is a SPI RX refactor pending at #6851, which may generate a conflict in this PR.

@chibaron

This comment has been minimized.

Copy link
Contributor Author

chibaron commented Oct 1, 2018

  1. Add cli command 'rx_bind'. This is the same function as frsky_bind. (Should I replace frsky_bind?)
    I prefer to use consolidated rx_bind, but rx_bind may be too broad... Sounds like it works for all receivers.
  1. Add cli setting value 'sfhss_spi_tx_id' and 'sfhss_spi_offset'. This is sfhss-bind parameter.
    Do we need _spi_ here?
    If this is to emphasize onboard receiver function, why not use rx_spi_bind (still too broad).

sfhss_spi_bind, sfhss_spi_tx_id and sfhss_spi_offset
or
sfhss_bind, sfhss_tx_id and sfhss_offset?

Frsky is
frsky_bind,frsky_spi_tx_is and frsky_spi_offset.

in the same way,
sfhss_bind,sfhss_spi_tx_is and sfhss_spi_offset?

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 1, 2018

Oh, then there is already an inconsistency... 😭

@chibaron chibaron closed this Oct 2, 2018

@chibaron chibaron deleted the chibaron:add_sfhss branch Oct 2, 2018

@chibaron chibaron restored the chibaron:add_sfhss branch Oct 2, 2018

@chibaron

This comment has been minimized.

Copy link
Contributor Author

chibaron commented Oct 2, 2018

mistake and deleted

@chibaron chibaron reopened this Oct 2, 2018

@chibaron

This comment has been minimized.

Copy link
Contributor Author

chibaron commented Oct 2, 2018

Should I remove the commit CRAZYBEEF3FR?

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 2, 2018

It doesn't build without the mods, right?

The best thing you can do at this point is to drop some features from CRAZYBEEF3FR so that it fits.

@chibaron

This comment has been minimized.

Copy link
Contributor Author

chibaron commented Oct 2, 2018

I can not decide, drop some features from CRAZYBEEF3FR.
I will remove the commit CRAZYBEEF3FR.

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 2, 2018

How are you going to let CRAZYBEEF3FR target to build then?

@chibaron

This comment has been minimized.

Copy link
Contributor Author

chibaron commented Oct 2, 2018

I made a debug in version 3.5.1 and at the master branch code was overflowing.
but I found a #undef... that removes functionality in target.h.
I think modify this code to go well.

@mikeller
Copy link
Member

mikeller left a comment

This looks interesting, and is a valuable addition.

Please fix according to my comments.

#ifdef USE_RX_FRSKY_SPI
CLI_COMMAND_DEF("frsky_bind", "initiate binding for FrSky SPI RX", NULL, cliFrSkyBind),
#ifdef USE_RX_BIND
CLI_COMMAND_DEF("frsky_bind", "initiate binding for FrSky SPI RX", NULL, cliRxBind),

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

I always wanted to change this to be applicable to more than just the FrSky SPI RX, and rename it to bind (i.e. make FlySky support it too). I guess the renaming to bind should be done now.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

change to bind

@@ -2560,13 +2561,14 @@ static void cliBeeper(char *cmdline)
}
#endif

#ifdef USE_RX_FRSKY_SPI
void cliFrSkyBind(char *cmdline){
#ifdef USE_RX_BIND

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

Please add a second conditional USE_CC2500_RX_BIND here guarding the respective case statements, so that other RX bind methods can be added.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

@@ -1112,6 +1114,10 @@ const clivalue_t valueTable[] = {
{ "frsky_spi_bind_hop_data", VAR_UINT8 | MASTER_VALUE | MODE_ARRAY, .config.array.length = 50, PG_RX_FRSKY_SPI_CONFIG, offsetof(rxFrSkySpiConfig_t, bindHopData) },
{ "frsky_x_rx_num", VAR_UINT8 | MASTER_VALUE, .config.minmax = { 0, 255 }, PG_RX_FRSKY_SPI_CONFIG, offsetof(rxFrSkySpiConfig_t, rxNum) },
{ "frsky_spi_use_external_adc", VAR_UINT8 | MASTER_VALUE | MODE_LOOKUP, .config.lookup = { TABLE_OFF_ON }, PG_RX_FRSKY_SPI_CONFIG, offsetof(rxFrSkySpiConfig_t, useExternalAdc) },
#endif
#ifdef USE_RX_SFHSS_SPI
{ "sfhss_tx_id", VAR_UINT8 | MASTER_VALUE | MODE_ARRAY, .config.array.length = 2, PG_RX_SFHSS_SPI_CONFIG, offsetof(rxSfhssSpiConfig_t, bindTxId) },

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

What is the reason for not using rsky_spi_tx_id, frsky_spi_offset for this, and does the user get any value out of this?

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

Change SFHSS bind parameter rxSfhssSpiConfig to rxFrSkySpiConfig.
Remove rxSfhssSpiConfig.

static IO_t rxLnaEnPin;
static IO_t antSelPin;
#endif
static int16_t rssiDbm;

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

This should respect USE_RX_FRSKY_SPI_TELEMETRY.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

Do you mean rssiDbm?
In that case I think that it is better to separate TELEMETRY and RSSI.
RSSI can be displayed with OSD.

This comment has been minimized.

@mikeller

mikeller Oct 3, 2018

Member

Ok, fair enough. But in this case the calls to cc2500setRssiDbm() should also not be guarded by USE_RX_FRSKY_SPI_TELEMETRY, or displaying RSSI becomes meaningless.

static bool ledIsOn=true;
static timeMs_t ledBlinkMs = 0;

if ( (ledBlinkMs + blinkms) > millis() ){

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

No spaces directly on the inside of (.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

cc2500WriteReg(CC2500_2E_TEST0, 0x0B);
cc2500WriteReg(CC2500_3E_PATABLE, 0xFF);

for(unsigned c = 0;c < 30; c++){

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

Space between ) and {.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

fixed

@@ -133,12 +133,13 @@
#define USE_RX_FRSKY_SPI_D
#define USE_RX_FRSKY_SPI_X
#define USE_RX_FRSKY_SPI_TELEMETRY
#define USE_RX_SFHSS_SPI

This comment has been minimized.

@mikeller

mikeller Oct 2, 2018

Member

If this target is overflowing try uncommenting the #undef USE_ESC_SENSOR_INFO above.

This comment has been minimized.

@chibaron

chibaron Oct 2, 2018

Author Contributor

uncomment
#undef USE_PINIO
#undef USE_PINIOBOX
#undef USE_ESC_SENSOR_INFO

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 2, 2018

@chibaron I think the TravisCI is stuck. Your next push will likely to resume it.

static IO_t rxLnaEnPin;
static IO_t antSelPin;
#endif
static int16_t rssiDbm;

This comment has been minimized.

@mikeller

mikeller Oct 3, 2018

Member

Ok, fair enough. But in this case the calls to cc2500setRssiDbm() should also not be guarded by USE_RX_FRSKY_SPI_TELEMETRY, or displaying RSSI becomes meaningless.

return false;
}
ccLen = cc2500ReadReg(CC2500_3B_RXBYTES | CC2500_READ_BURST) & 0x7F;
if (ccLen < SFHSS_PACKET_LEN){

This comment has been minimized.

@mikeller

mikeller Oct 3, 2018

Member

There still are some of instances of ){.

chibaron added some commits Oct 3, 2018

nextChannel(1);
cc2500setRssiDbm(packet[18]);

This comment has been minimized.

@mikeller

mikeller Oct 5, 2018

Member

Not sure if this is right (the underlying FrSky D protocol has only been reverse engineered), we probably should still check (packet[3] % 4) == 2) before setting this.

This comment has been minimized.

@chibaron

chibaron Oct 5, 2018

Author Contributor

I think RSSI has nothing to do with the protocol. CC2500 adds RSSI to all packets. Frsky_X gets RSSI for all packets.

          Air                        SPI
TX ----[Packet]----> CC2500 ----[Packet+RSSI]---> MPU

The change may become faster because the call to setRssi () is quadrupled.
Should I add check (packet[3] % 4) == 2) ?

This comment has been minimized.

@mikeller

mikeller Oct 6, 2018

Member

No, that should be fine then.

@mikeller mikeller added this to the 4.0 milestone Oct 6, 2018

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Oct 6, 2018

@jflyper: Have your concerns with this pull request been addressed?

outdated

@mikeller mikeller merged commit cc820d9 into betaflight:master Oct 7, 2018

@jflyper

This comment has been minimized.

Copy link
Contributor

jflyper commented Oct 7, 2018

@mikeller I'm okay.

@chibaron Good work!!! 👍

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Oct 7, 2018

Aand now CRAZYBEEF3FR exploded! 😜

Fixed in #6900.

@chibaron chibaron deleted the chibaron:add_sfhss branch Oct 10, 2018

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.