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 SPI SPEKTRUM protocol #7210

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@phobos-
Copy link
Contributor

phobos- commented Dec 11, 2018

This PR adds cyrf6936 SPI driver and spektrum dsm2 / dsmx 11ms / 22ms protocols with simple telemetry.

@mikeller
Copy link
Member

mikeller left a comment

This looks very interesting!

However, can you please split it into two pull requests: One to add the RX SPI functionality, another one to add the new target.

@@ -31,10 +31,6 @@

#include "rx/rx_spi.h"

#ifndef RX_SPI_LED_PIN

This comment has been minimized.

@mikeller

mikeller Dec 11, 2018

Member

This should not be removed.

This comment has been minimized.

@phobos-

phobos- Dec 11, 2018

Author Contributor

I moved it to src/main/target/common_defaults_post.h, similarly to #7135
I'll open a separate PR for this change.

Edit: #7211

#define DSM_TELEMETRY_TIMEOUT_US 1500
#define DSM_TELEMETRY_TIME_US 176000

typedef enum {

This comment has been minimized.

@mikeller

mikeller Dec 11, 2018

Member

Are all of these type definitions and defines meant to be used outside of cyrf6936_spektrum.c? If not they should not be part of the header file / API

This comment has been minimized.

@phobos-

phobos- Dec 11, 2018

Author Contributor

No, I'll refactor them into cyrf6936_spektrum.c.

This comment has been minimized.

@phobos-

phobos- Dec 11, 2018

Author Contributor

Actually, I just started refactoring, but noticed that I use them in the unit test.

This comment has been minimized.

@mikeller

mikeller Dec 11, 2018

Member

I'd go for defining the ones that are needed in the unit test in the test itself - I don't think that things used for testing only should be part of the API.

@@ -26,6 +26,9 @@
#elif defined(CRAZYBEEF3DX)
#define TARGET_BOARD_IDENTIFIER "CBDX"
#define USBD_PRODUCT_STRING "CrazyBee F3 DX"
#elif defined(CRAZYBEEF3DXSPI)
#define TARGET_BOARD_IDENTIFIER "CBDS"

This comment has been minimized.

@mikeller

mikeller Dec 11, 2018

Member

Is this actually a board that is being manufactured / sold?

This comment has been minimized.

@phobos-

phobos- Dec 11, 2018

Author Contributor

No, I believe I own the only two bards in existence as of now.

This comment has been minimized.

@mikeller

mikeller Dec 11, 2018

Member

Probably a good idea to add this to a different target instead:

  • there is already a CRAZYBEEF3 target with a Spektrum RX - having 2 of them to choose from will be confusing for users;
  • adding this to a target that has SPI pins exposed on a header (OMNIBUSF4FW, BEEROTORF4, MATEKFxx?, ...) will make it easier for other users to add their homebrew SPI RX mods.

You might want to talk to @githubDLG though, he might be interested in producing a board with a Spektrum SPI RX.

This comment has been minimized.

@githubDLG

githubDLG Dec 12, 2018

Contributor

hehe, hi, mikeller and phobos~~~
Actually, we and phobos have already work together on this spi dsmx for a while.
And, the previous crazybee f3 dx is a intergrated serial rx, while this new crazybee f3 dx spi will acess the rfic directly via spi.
Including the two variant in one target maybe a natually thing, but seperate crazybee f3 dx spi to another target will also acceptable then.
And, anyway , thanks for the work that your guys did!!! @mikeller @phobos-

This comment has been minimized.

@mikeller

mikeller Dec 15, 2018

Member

@githubDLG: Good to hear that you are working together. I think we will have to be careful with how we are naming the target with Spektrum SPI RX - it's probably not possible to have a target that will automatically detect if the board uses SPI or an UART, so we will end up with two targets, and it will be important to make sure users know which target to use.

This comment has been minimized.

@phobos-

phobos- Dec 15, 2018

Author Contributor

I'll open a spearate PR with target specific changes, and we can discuss it there. Since the board is not produced target definition is very flexible. It is up to you @mikeller and @githubDLG , but please agree on some standard so that I can work on it and prepare the code.

@mikeller right now it is a separate variant CRAZYBEEF3DXSPI (versus CRAZYBEEF3DX for serial rx). Maybe the SPI target should use the same pattern (two letters after F3) but with different characters?

@githubDLG

This comment has been minimized.

Copy link
Contributor

githubDLG commented Dec 12, 2018

hi, guys~~~

@phobos- phobos- force-pushed the phobos-:spi_dsm branch from 24a808d to a3af20f Dec 12, 2018

@phobos-

This comment has been minimized.

Copy link
Contributor Author

phobos- commented Dec 12, 2018

@mikeller removed target specific code and moved relevant structures out of the API header.

How should I proceed with adding a new target? I've never done a PR that depends on another PR. If I add a PR with target specific code the CI will fail untill it is rebased against this PR.

@githubDLG

This comment has been minimized.

Copy link
Contributor

githubDLG commented Dec 12, 2018

maybe need one by one.....

(IS_DSMX(protocol) && packet[0] == id[2] && packet[1] == id[3]))

typedef struct spektrumConfig_s {
dsm_protocol_e protocol;

This comment has been minimized.

@mikeller

mikeller Dec 15, 2018

Member

Use an uint8_t here - enums are stored as 32 bit, which isn't needed in the parameter group.

This comment has been minimized.

@phobos-

phobos- Dec 15, 2018

Author Contributor

Done.

@phobos- phobos- force-pushed the phobos-:spi_dsm branch 2 times, most recently from 02a91e0 to 012c985 Dec 15, 2018

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Dec 15, 2018

@phobos-: There's more than one way to get this done. One by one is fine. Separately works as well, there's nothing wrong with failing checks if they are failing for a good reason. Or you can create a branch for your new target off the branch for this change, so the checks will be fine now, and your second pull request will show only the changes in your second branch as soon as this gets merged.

@mikeller mikeller added this to the 4.0 milestone Dec 15, 2018

#define DSM_TELEMETRY_TIME_US 176000

typedef struct spektrumConfig_s {
uint8_t protocol;

This comment has been minimized.

@mikeller

mikeller Dec 15, 2018

Member

Sorry to come back to this over and over again, but there's a lot of ground to cover in this review.

These settings should be exposed in src/main/interface/settings.c. Even if users are very unlikely to ever want to set them manually, if they are exposed they will be added to the diff, and then can be re-applied after a firmware update. Otherwise the user will have to re-bind the RX every time they update the firmware.

This comment has been minimized.

@phobos-

phobos- Dec 16, 2018

Author Contributor

No worries, I actually added it for debugging purposes, but then removed it for the final pull request....
Updated with settings.c changes.

@phobos- phobos- force-pushed the phobos-:spi_dsm branch from 012c985 to 6651baa Dec 16, 2018

@mikeller mikeller merged commit ee0d86f into betaflight:master Dec 17, 2018

@phobos- phobos- deleted the phobos-:spi_dsm branch Dec 17, 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.