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

Introduce per peripheral dma channel spec option #6837

Merged
merged 1 commit into from Dec 30, 2018

Conversation

Projects
None yet
4 participants
@jflyper
Copy link
Contributor

jflyper commented Sep 24, 2018

Provides configurability of DMA stream/channel specification, which is the next blocking factor toward generic target.

Per #6803 (comment)

Show resolved Hide resolved src/main/drivers/sdcard.c Outdated
@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Sep 24, 2018

This looks good to me.

We probably want to use 0 as the default and not allow -1 as a setting for any resource that requires DMA (but we can fail any option to -1 during initialisation if a conflict is detected). In cases like SD card, -1 can be allowed, and used to replace the use_dma parameter.

@stale

This comment has been minimized.

Copy link

stale bot commented Oct 25, 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 Oct 25, 2018

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Oct 25, 2018

@jflyper: Should this stay open?

@stale stale bot removed the Inactive label Oct 25, 2018

@jflyper

This comment has been minimized.

Copy link
Contributor Author

jflyper commented Oct 25, 2018

Yeah, working on it intermittently ... but ...

I'm beginning to think if this is the right way to go.

Conversion of target.h was mostly done systematically by a script and hard-coded ones by hand. In a course, I had to lookup the original DMAy_Streamx value in the table, place it in the source code, then had to lookup the original format value in the original source code to match CLI dma output to dmaopt-style value. I even started to leave the old format in comments. If it was in, say, in a format like DMASPEC(2, 5, 0), it would have been super easy to see the spec and the result.

@mikeller mikeller added the Pinned label Oct 25, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-dma-channel-spec branch from d7a5a39 to f606345 Dec 18, 2018

@jflyper jflyper force-pushed the jflyper:bfdev-poc-dma-channel-spec branch from 91b30aa to 5a523d5 Dec 28, 2018

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

@jflyper jflyper force-pushed the jflyper:bfdev-poc-dma-channel-spec branch from 943112f to c2dcc5d Dec 29, 2018

@jflyper

This comment has been minimized.

Copy link
Contributor Author

jflyper commented Dec 29, 2018

Since there are no targets that is using DMA for SD card, UART and MAX7456, it leaves us with ADC and SDIO.

SDIO is only used for a single target with limited distribution and the driver still requires additional heavy work, as it still has DMAx_Streamy type constant embedded in the .c files.

Above observation leaves us ADC as a primary and heavy user of the DMA atm.

ADC driver side tests
F4 (Omnibus F4 gen1)

  • ADC1 ADC1_DMA_OPT 0
  • ADC1 ADC2_DMA_OPT 1 (Compatible with pre-4.0 default)
  • ADC2 ADC2_DMA_OPT 0
  • ADC2 ADC2_DMA_OPT 1 (Compatible with pre-4.0 default)

F7 (Omnibus F7 V2)

  • ADC1 ADC1_DMA_OPT 0
  • ADC1 ADC2_DMA_OPT 1 (Compatible with pre-4.0 default)
  • ADC2 ADC2_DMA_OPT 0
  • ADC2 ADC2_DMA_OPT 1 (Compatible with pre-4.0 default)

Target conversion tests

  • Omnibus F4 (gen1)
  • Matek F411RX
  • Omnibus F7 V2
@ledvinap

This comment has been minimized.

Copy link
Contributor

ledvinap commented Dec 29, 2018

Using DMA for ADC is overkill. And rewrite using ISR should be quite easy (in simplest case just copying data to current DMA buffer)

@jflyper

This comment has been minimized.

Copy link
Contributor Author

jflyper commented Dec 29, 2018

Yeap. It has been pointed out earlier last week by joelucid. Will do it sometime before 4.0 ;)

@mikeller
Copy link
Member

mikeller left a comment

Looking good, I can't wait to get this piece of the 'unified target' puzzle in place. Just some cosmetic remarks.

Show resolved Hide resolved src/main/msc/usbd_storage_sdio.c
@@ -124,6 +124,10 @@
#define SRAM2
#endif

#if defined(STM32F4) || defined(STM32F7)

This comment has been minimized.

@mikeller

mikeller Dec 29, 2018

Member

Probably more consistent if we add this to the sections for STM32F4 and STM32F7 above.

This comment has been minimized.

@jflyper

jflyper Dec 30, 2018

Author Contributor

The above section is ifdef-else-endif for a scheduler oriented constant pair. It doesn't look good to add another def only to ifdef section. Instead, I've moved up the USE_DMA_SPEC immediately below the scheduler constant section for an improved readability.

This comment has been minimized.

@mikeller

mikeller Dec 30, 2018

Member

No, I meant the two #ifdef STM32F4 and #ifdef STM32F7 blocks above that (https://github.com/betaflight/betaflight/pull/6837/files#diff-016d7c2e306c7201ea0ec246bc617247R50). The scheduler related constant pair is different as it needs an #else case, whereas other conditionals that are specific to MCU type should go into the blocks above in my opinion.

This comment has been minimized.

@jflyper

jflyper Dec 30, 2018

Author Contributor

Got it 👍

@jflyper jflyper force-pushed the jflyper:bfdev-poc-dma-channel-spec branch from 0f14396 to fe182bb Dec 30, 2018

@mikeller mikeller merged commit be797d1 into betaflight:master Dec 30, 2018

@jflyper jflyper deleted the jflyper:bfdev-poc-dma-channel-spec branch Dec 30, 2018

@Asizon

This comment has been minimized.

Copy link
Contributor

Asizon commented on fe182bb Dec 30, 2018

unfortunately, betaflight does not tell me voltage data after this commit.Im using telemetry via softserial

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.