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

Configurable ACC/GYRO #5868

Merged
merged 5 commits into from Sep 13, 2018

Conversation

Projects
None yet
3 participants
@jflyper
Copy link
Contributor

commented May 10, 2018

Provides configurable ACC/GYRO.

  • All targets were modified to use dual gyro by default. At this point, USE_DUAL_GYRO has been diminished.
  • Then F3 targets where modified to use single gyro, by conditional-izing F4 and F7 case with USE_MULTI_GYRO (name deliberately chosen different from USE_DUAL_GYRO to avoid illusions in change diff set.
  • Acc/gyros are identified not by device type names, but GYRO_{1,2} and ACC_{1,2}` prefixes.

[Following is the initial first comment]
This PR presents a PoC mods to make all targets to USE_DUAL_GYRO, to untangle the web of conditionals in acc/gyro system toward universal/generic target.

After this conversion, USE_DUAL_GYRO conditionals can be removed, and should be much easier to refactor and/or converted to configurable system.

Thoughts?

@mikeller
Copy link
Member

left a comment

I like the idea of making everything dual (multi?) gyro capable.
What we have to do is take this and check how much additional flash space this will cost us, and if there is an easy way to cut down on extra flash space usage without reintroducing the tangle, and put this into a USE_SINGLE_GYRO define to be used on F3.

@@ -2443,7 +2443,7 @@ static void cliPrintGyroRegisters(uint8_t whichSensor)

static void cliDumpGyroRegisters(char *cmdline)
{
#ifdef USE_DUAL_GYRO
#ifdef GYRO_2_CS_PIN

This comment has been minimized.

Copy link
@mikeller

mikeller May 10, 2018

Member

Not sure if it's a good idea to replace the USE_DUAL_GYRO with a pin define that we know will have to go again before we are ready for universal targets.

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch 3 times, most recently from 704e8f4 to 7e8d713 May 16, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch 4 times, most recently from 291822e to c07687e May 21, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch 3 times, most recently from 9e85db4 to af80445 May 28, 2018

@jflyper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

Alignment configuration for acc/gyro in target.h will be based occurrence number (1 or 2), as opposed to device type based one. This will force users of some target with multiple acc/gyro possibilities to perform post flashing configuration of sensor alignment.

Following targets are known to have this property.

  • FURYF3
  • MATEKF405
  • MATEKF722
  • SIRINFPV

Note that F4 and F7 targets are less problematic as these will go generic target with 4.0.

@jflyper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2018

Some F4/F7 targets only configures I2C acc/gyro, which can not be handled by the current USE_MULTI_GYRO facility which assumes all F4/F7 targets have SPI based cc/gyro.

Following targets are known to have this property:

  • CRAZYFLIE2
  • NUCLEOF7
@mikeller

This comment has been minimized.

Copy link
Member

commented Jun 2, 2018

This will force users of some target with multiple acc/gyro possibilities to perform post flashing configuration of sensor alignment.

If universal targets aren't ready when this is released, we can always use targetConfiguration() to do this automatically.

@jflyper

This comment has been minimized.

Copy link
Contributor Author

commented Jun 2, 2018

This will force users of some target with multiple acc/gyro possibilities to perform post flashing configuration of sensor alignment.

If universal targets aren't ready when this is released, we can always use targetConfiguration() to do this automatically.

Turned out that the acc/gyro chip alignment problem can not be handled with targetConfiguration(), as the alignment can only be determined after the chip is identified.

I think this is just one of many target dependent configuration that we have to solve before the generic target can be deployed in wide.

@jflyper jflyper changed the title PoC Migrate all targets to `USE_DUAL_GYRO` Configurable ACC/GYRO Jun 2, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch from fd501c6 to cd1f6d3 Jun 2, 2018

@jflyper jflyper added Review Needed and removed In Development labels Jun 2, 2018

@@ -75,6 +75,7 @@ static bool mpu9250SpiSlowReadRegisterBuffer(const busDevice_t *bus, uint8_t reg
return true;
}

#if 0 // XXX Not used at all. Debugging aid?

This comment has been minimized.

Copy link
@mikeller

mikeller Jun 3, 2018

Member

Do we really need nested #if 0 here? Should this just be removed, considering the comment says it doesn't work?

This comment has been minimized.

Copy link
@jflyper

jflyper Aug 31, 2018

Author Contributor

I think removal of the mpu9250SpiResetGyro and related (including mpuConfiguration_s.resetFn) better go into another PR, after finding out what was actually intended with the resetFn.

This comment has been minimized.

Copy link
@mikeller

mikeller Sep 3, 2018

Member

Then please convert this into a named #if defined(), so that it can at least be built with a command line option. Keeping this in as decoration serves no purpose.

@@ -3723,6 +3724,8 @@ const cliResourceValue_t resourceTable[] = {
#ifdef USE_RX_SPI
DEFS( OWNER_RX_SPI_CS, PG_RX_SPI_CONFIG, rxSpiConfig_t, csnTag ),
#endif
#define PG_ARRAY_OFFSET(type, index, member) (index * sizeof(type) + offsetof(type, member))

This comment has been minimized.

Copy link
@mikeller

mikeller Jun 3, 2018

Member

Is this meant to be slotted in here?

#include "pg/pg.h"
#include "drivers/io_types.h"
#include "drivers/sensor.h"
#include "sensors/gyro.h"

This comment has been minimized.

Copy link
@mikeller

mikeller Jun 3, 2018

Member

You can't include this here.

sensor_align_e align;
} gyroDeviceConfig_t;

#ifdef USE_MULTI_GYRO

This comment has been minimized.

Copy link
@mikeller

mikeller Jun 3, 2018

Member

This will never be defined here, can as well eliminate it.

#define MPU_INT_EXTI PC4
#define USE_EXTI
#define USE_GYRO_EXTI
#define GYRO_1_EXTI_PINPC4

This comment has been minimized.

Copy link
@mikeller

mikeller Jun 3, 2018

Member

Is this a typo? There's a whole bunch of GYRO_1_EXTI_PINPxx that look like search / replace gone bad.

@jflyper jflyper referenced this pull request Jun 17, 2018

Closed

adding helio target #6135

@stale

This comment was marked as resolved.

Copy link

commented Jul 3, 2018

This issue / pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@stale stale bot added the Inactive label Jul 3, 2018

@stale

This comment was marked as resolved.

Copy link

commented Jul 10, 2018

Automatically closing as inactive.

@stale stale bot closed this Jul 10, 2018

@jflyper jflyper reopened this Aug 31, 2018

@stale stale bot removed the Inactive label Aug 31, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch from cd1f6d3 to 5b2b8ea Aug 31, 2018

@jflyper jflyper added this to the 4.0 milestone Aug 31, 2018

@jflyper

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

@MJ666 You may want to start assessing impact of this PR for targets that does run time revision detection.

@MJ666

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2018

@jflyper not sure what you have in mind here? Basically the AF(NG) F3, F4 and F7 targets are using hardware detection. This is mainly for different configurations like brushed vs. brushless or RX vs. no RX. Only for the F3 it is handling the Gyro (I2C MPU6050 vs. SPI MPU6500/MPU9250)) and LED pins. The F4 variants only come with an SPI MPU6500/MPU9250. There also variants for the F7 out there with different SPI Gyro chips (MPU6500/MPU9250/ICM20602). The sensor orientation will be always the same and never change between Gyro chips.

@@ -75,6 +75,7 @@ static bool mpu9250SpiSlowReadRegisterBuffer(const busDevice_t *bus, uint8_t reg
return true;
}

#if 0 // XXX Not used at all. Debugging aid?

This comment has been minimized.

Copy link
@mikeller

mikeller Sep 3, 2018

Member

Then please convert this into a named #if defined(), so that it can at least be built with a command line option. Keeping this in as decoration serves no purpose.

@jflyper

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2018

@MJ666
Hardware variant detection and associated automatic default configuration, is likely to be gone with generic target transition. Providers of this facility have to be aware of this move, and are expected to take appropriate actions.

The easiest way is to provide per hardware post-flash configuration files. (Note that every F4 and F7 targets will require at least one such file.)

Reinstating automatic variant detection under generic target scheme would require something innovative to happen.

@MJ666

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

Removing the hardware detection completely is in my view not a good plan. In some cases the wrong firmware may semi brick the related FC and this will generate a lot of additional support requests. This will definitely the case for the AF F3 V1 and V2.

What about the following strategy? We can implement some generic hardware detection code. This will be enabled during the load of the configuration with the definition of the pin(s) and allow the configurator to get the hardware revision number via MSP and will load an additional set of version related settings defined in the configuration file. What is the plan for flashing the firmware an loading the config. Will this be done in a single step. Likely the firmware need to be rebooted after flashing anyhow before loading the config?

How far we are with the generic targets is the STM32F7X2 with NERO already working? Does it make sense to port the AFNGF7 already and start more testing? Ist there any fork to contribute for the F3 or F4 already.

@mikeller

This comment has been minimized.

Copy link
Member

commented Sep 8, 2018

@MJ666: Can you please elaborate a bit on this:

  • where do you see the risk of boards being semi-bricked if hardware detection is removed? Most likely, the best way around this for generic targets will be to ensure that this semi-bricking does not happen any more;
  • how would you want to do for 'generic hardware detection'? Keeping in mind the fact that for generic targets, no pins at all (except for USB VCP) will be defined by the hardware, anything but blindly probing pins will not work before a configuration is loaded. And my suspicion is that probing pins will only increasing the chances of semi-bricking boards, if anything.

@jflyper jflyper force-pushed the jflyper:bfdev-poc-migrate-to-dual-gyro branch from 3edd50b to 59db3f6 Sep 10, 2018

@MJ666

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@jflyper the risk fro semi bricking i mostly see fro the Alienflight F3 pins are used in a different way for V1 and V2. V1 has an I2C gyro and V2 has an SPI gyro. the LED's are on different pins for both targets and there is some collision between both targets.
The config can define the hardware detection pins and the configurator can read the hardware revison and configure the related config?
An second option would be the use the manufacturer ID to select the proper config. Here only the fiirst time flashing is important.

@mikeller

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

@MJ666: I see. For F3 it is unlikely that we well move to fully generic targets, but we'll probably still attempt to leverage the changes we make for F4 / F7 to reduce the F3 targets down to a list of 'unified' targets for each unique combination of drivers, with the target specific pin assignments being defined in the configuration.

As for doing the hardware revision evaluation on the configurator side, this is definitely possible, but unless the hardware detection pin is hardcoded (i.e. not supplied by the target specific configuration), this will require a multi stage configuration where other targets will work with a single stage configuration.

The manufacturer / board id were introduced with the intention to help users at some stage to not have to remember the type of their board when updating the firmware. Leveraging this for target variants would change their status from variants to individual targets (which could be done even without leveraging the manufacturer / board id), but at the same time making it a no brainer for the user to find the right target configuration to update.

@mikeller mikeller merged commit 8207dab into betaflight:master Sep 13, 2018

1 check passed

Travis CI - Pull Request Build Passed
Details

@jflyper jflyper deleted the jflyper:bfdev-poc-migrate-to-dual-gyro branch Sep 14, 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.