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

F3 and X-Vert support #2113

Merged
merged 16 commits into from
Oct 6, 2017
Merged

Conversation

kevindehecker
Copy link
Contributor

@kevindehecker kevindehecker commented Sep 28, 2017

Adds support for:

  1. STM32F373x
  2. The Horizon Hobbies Eflite X-Vert VTOL (based on the STM32F373xC)
  3. STM32F303x (preliminary)
  4. The F3 Discovery board (based on the STM32F303)

All are based on Chibios. In order to fit the RAM memory footprint on the 32K RAM of the STM32F37, the thread working area sizes have been reduced. It all seems to work without fault, so I don't think it is necessary to increase the size for larger chips such as the F7.
Another thing is that the PPRZ_TRIG_CONST needed to be defined as CONST. This placed the sin integer lookup table on flash. The other defines such as PPRZ_TRIG_INT_COMPR_FLASH do not seem to have any effect. Of course, the new define PPRZ_TRIG_INT_USE_FLOAT also works.

The x-vert has been flight tested. It can hover manually. Further steps (autonomous, forward flight) are planned. A wiki page about the x-vert is in the works.

Known problems:
The X-vert ESCs provide motor feedback, but this is not yet read out
The X-vert may have a current sensor, but it is not yet read out
The X-vert board has an CYRF chip for radio control, but this is not supported yet in this PR.
The Chibios stm32F37 implementation has a problem with the hard FPU. When enabled, totally weird results happen.

@podhrmic
Copy link
Member

I ll look over it over the weekend.

## FPU on F3

##TODO: there is some bug when using the hard FPU unit!!
USE_FPU=no
Copy link
Member

Choose a reason for hiding this comment

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

can you file a bug report at ChibiOS forum about it? Giovanni is typically fairly responsive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's probably something with wrong settings for the minimal float number. Was planning to look into it a bit further myself later.

#endif

adcgrpcfg.circular = TRUE;
adcgrpcfg.num_channels = ADC_NUM_CHANNELS;
adcgrpcfg.end_cb = adc1callback;
adcgrpcfg.error_cb = adcerrorcallback;
#if defined(__STM32F373xC_H)
Copy link
Member

Choose a reason for hiding this comment

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

it would be nice to have a better way of specifying the driver than adding ifdefs, perhaps separate files for each chibios arch - but that is indeed out of scope of this PR

(uint8_t*)i->dma_buf, (size_t)(t->len_r),
tmo);
memcpy((void*)t->buf, i->dma_buf, (size_t)(t->len_r));
(I2CDriver *)p->reg_addr,
Copy link
Member

Choose a reason for hiding this comment

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

stylecheck pls?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix_code_style is actually what caused this...

Copy link
Member

Choose a reason for hiding this comment

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

I wrote this part of the code. The code_style script should be correct, it is probably my VIM settings for aligning multi-line parameters that are a bit different.

@@ -198,7 +198,7 @@ static struct i2c_init i2c1_init_s = {
struct i2c_errors i2c1_errors;
// Thread
static __attribute__((noreturn)) void thd_i2c1(void *arg);
static THD_WORKING_AREA(wa_thd_i2c1, 1024);
static THD_WORKING_AREA(wa_thd_i2c1, 128);
Copy link
Member

Choose a reason for hiding this comment

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

I would be worried that this might overflow for larger i2c buffers - could you add ifndef guard and specify this number in the board file to override the default?

Copy link
Contributor Author

@kevindehecker kevindehecker Oct 2, 2017

Choose a reason for hiding this comment

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

I don't think that's how it works. The data goes in different buffers. E.g. the ublox I2C gps (used in this PR) actually is a rather large buffer user compared to most other sensors, it does not seem to be a problem. Also, the chibios examples themselves use this number. I think they were oversized in pprz. Of course could add the ifdef overrides if still required, but think these numbers here should be the default.

I'm not sure though what happens when the buffer is to small, I may try that when I get the chance,
It would also be nice if someone with e.g. a Chimera can test this.

(same for all the other THD_WORKING_AREA's)

Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds reasonable. I think ChibiStudio plugin for Eclipse can tell you the used memory for each thread.

Copy link
Member

Choose a reason for hiding this comment

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

i2c buffers are global variables so not counted for the stack size. Only automatic variables (more or less the local variables) created by the thread will use the stack memory, a few pointers in the case of this driver. So I guess 128 is fine.

@@ -347,15 +346,15 @@ static __attribute__((noreturn)) void thd_spi1(void *arg)
}
}

static THD_WORKING_AREA(wa_thd_spi1, 1024);
static THD_WORKING_AREA(wa_thd_spi1, 256);
Copy link
Member

Choose a reason for hiding this comment

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

again - would be better to have this board-dependent with larger defaults.

} else {
ahrs_fc.weight = 1.0;
}
static struct FloatVect3 filtered_gravity_measurement = {0., 0., 0.};
Copy link
Member

Choose a reason for hiding this comment

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

isn't pprz style with 2 spaces? Pls run the stylecheck script on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, all these changes were done by fix_code_style

Copy link
Contributor Author

@kevindehecker kevindehecker Oct 2, 2017

Choose a reason for hiding this comment

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

If I run it again, it gives the same results. It does give some weird error warning though:
~/paparazzi$./fix_code_style.sh sw/airborne/subsystems/ahrs/ahrs_float_cmpl.c
Using astyle version 2.05.1
(standard_in) 1: syntax error
./fix_code_style.sh: line 18: [: -eq: unary operator expected
Unchanged sw/airborne/subsystems/ahrs/ahrs_float_cmpl.c

Copy link
Member

Choose a reason for hiding this comment

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

Don't know why the script does the warning, maybe @flixr knows more?

However, the file broke the style check - compare lines 206 const struct FloatVect3 vel_tangential_body = before and after. Clearly the correct style is before applying the style check.

Pls revert and check your changes manually.

Also there is a typo at line 212 - the end bracket should be } instead of )


}
const float cphi = cosf(ltp_to_imu_euler.phi);
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason for changes here (they aren't mentioned in the PR description)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird, all this is done by fix_code_style. I only did the two minor changes in commit 8cdc6d7

Copy link
Member

Choose a reason for hiding this comment

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

Ok makes sense

@@ -1,10 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE QtCreatorProject>
<!-- Written by QtCreator 4.1.0, 2017-01-23T23:23:44. -->
<!-- Written by QtCreator 4.4.0, 2017-09-08T21:31:25. -->
Copy link
Member

Choose a reason for hiding this comment

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

this file should probably be a part of .gitignore

Copy link
Contributor Author

@kevindehecker kevindehecker Oct 2, 2017

Choose a reason for hiding this comment

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

Actually, it should not. The template is used to generate a valid qtc project.user. Unfortunately it currently means that the template has to be updated when qtc changes. Looking for a better solution, but nothing as of yet.
(other than that, I can recommend qt over eclipse. It only shows you the files that are actually used)

@@ -3,55 +3,71 @@
#It finds the correct VPATH to each source file from make, and then feeds all info to gcc -MM.
import sys
import os
from os import path, getenv
from os import path, getenv, environ
Copy link
Member

Choose a reason for hiding this comment

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

how are these changes related to PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order to get a good qt project with a chibios based board, some changes were needed. Since they are in a clear seperate commit I did not bother creating a separate PR

while line:
line=ap_srcs_list.readline()
for line in ap_srcs_list:
#line=ap_srcs_list.readline()
Copy link
Member

Choose a reason for hiding this comment

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

remove the comment?

@podhrmic
Copy link
Member

podhrmic commented Oct 2, 2017

@kevindehecker the only remaining thing is the code style for ahrs.c file (see my comment above), the rest looks good (at least for the time being).

@kevindehecker
Copy link
Contributor Author

Done!

@podhrmic
Copy link
Member

podhrmic commented Oct 5, 2017

LGTM. @gautierhattenberger or @flixr do you want to add anything or good to merge?

@@ -340,9 +340,17 @@ static inline void stabilization_indi_calc_cmd(int32_t indi_commands[], struct I
indi.u_in.r = indi.u[2].o[0] + indi.du.r;

//bound the total control input
#ifdef INDI_FULL_AUTHORITY
Copy link
Member

Choose a reason for hiding this comment

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

This new option should be documented in the module xml....

Copy link
Member

Choose a reason for hiding this comment

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

Also please use #if FLAG instead of #ifdef FLAG according to http://docs.paparazziuav.org/latest/stylec.html#stylecpp

</module>
</firmware>

<modules main_freq="512">
Copy link
Member

Choose a reason for hiding this comment

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

would be nice to move this modules to the firmware section

@@ -198,7 +198,7 @@ static struct i2c_init i2c1_init_s = {
struct i2c_errors i2c1_errors;
// Thread
static __attribute__((noreturn)) void thd_i2c1(void *arg);
static THD_WORKING_AREA(wa_thd_i2c1, 1024);
static THD_WORKING_AREA(wa_thd_i2c1, 128);
Copy link
Member

Choose a reason for hiding this comment

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

i2c buffers are global variables so not counted for the stack size. Only automatic variables (more or less the local variables) created by the thread will use the stack memory, a few pointers in the case of this driver. So I guess 128 is fine.

@@ -42,7 +42,7 @@
* Also include FBW on single MCU
*/
static void thd_ap(void *arg);
static THD_WORKING_AREA(wa_thd_ap, 8192);
THD_WORKING_AREA(wa_thd_ap, 1024);
Copy link
Member

Choose a reason for hiding this comment

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

Here, I don't agree to reduce the stack size. We are running some estimation filters that requires some memory. So I suggest to keep the 8K by default and make it configurable.

(uint8_t*)i->dma_buf, (size_t)(t->len_r),
tmo);
memcpy((void*)t->buf, i->dma_buf, (size_t)(t->len_r));
(I2CDriver *)p->reg_addr,
Copy link
Member

Choose a reason for hiding this comment

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

I wrote this part of the code. The code_style script should be correct, it is probably my VIM settings for aligning multi-line parameters that are a bit different.

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.

None yet

4 participants