Skip to content

TARGET_STM :USB device FS #3062

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 29, 2016
Merged

TARGET_STM :USB device FS #3062

merged 8 commits into from
Nov 29, 2016

Conversation

jamike
Copy link
Contributor

@jamike jamike commented Oct 18, 2016

Description

This brings the support of USB device for:
NUCLEO_F746Zx, NUCLEO_F446ZE, NUCLEO_F303ZE, NUCLEO_F207ZG, DISCO_L476VG.
Implementation relies on STM USB HAL.
The board NUCLEO_F401RE, NUCLEO_F429ZI, DISCO_F407VG
supports in F4 without STM USB HAL can also run with this implementation
define USB_STM_HAL in targets.json

Status

READY

Steps to test or reproduce

USB test can be passed.

0xc0170
0xc0170 reviewed Oct 26, 2016
View reviewed changes
features/unsupported/USBDevice/USBDevice/TARGET_STM/USBHAL_STM32.cpp

extern "C" {
/* this call at device reception completion on a Out Enpoint */
void HAL_PCD_DataOutStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you align this code block to the beginning of the line? Where are ths functions declared?

0xc0170
0xc0170 reviewed Oct 26, 2016
View reviewed changes
features/unsupported/USBDevice/USBDevice/TARGET_STM/USBHAL_STM32.cpp
}

void USBHAL::connect(void) {
NVIC_EnableIRQ(USBHAL_IRQn);
Copy link
Contributor

Choose a reason for hiding this comment

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

tabs - misaligned?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are below same problems, please look at the changeset .

0xc0170
0xc0170 reviewed Oct 26, 2016
View reviewed changes
features/unsupported/USBDevice/USBDevice/TARGET_STM/USBHAL_STM32.cpp
// TODO: Find out if this is correct behavior, or whether we are doing
// something else wrong
stallEndpoint(EP0IN);
//stallEndpoint(EP0OUT);
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code?

@sg-
Copy link
Contributor

sg- commented Nov 1, 2016

@jamike any updates about code style changes? Also need to enable CI in build_travis.py for all targets that are supporting USB

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 7, 2016

bump , any update?

@jamike jamike force-pushed the TARGET_STM_USBDEVICE_FS branch from b079c42 to f2d967e Compare November 7, 2016 16:22
@jamike
Copy link
Contributor Author

jamike commented Nov 8, 2016

update done.

@bridadan
Copy link
Contributor

bridadan commented Nov 8, 2016

Thanks for adding the library build step, could you also add the linking step? You can see how it was done for other platforms here: https://github.com/ARMmbed/mbed-os/blob/master/tools/build_travis.py#L163

@jamike jamike force-pushed the TARGET_STM_USBDEVICE_FS branch from a39eeca to 91ae6db Compare November 9, 2016 16:26
@jamike
Copy link
Contributor Author

jamike commented Nov 9, 2016

updated and rebased.

@sg-
Copy link
Contributor

sg- commented Nov 10, 2016

@mazimkhan @marhil01 CI status update check?

@sg-
Copy link
Contributor

sg- commented Nov 10, 2016

/morph test

@sg-
Copy link
Contributor

sg- commented Nov 10, 2016

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@sg- sg- added needs: CI and removed needs: work labels Nov 10, 2016
@mazimkhan
Copy link

Uvisor Build and Test passed http://e108747.cambridge.arm.com:8080/job/mbed-os/job/mbed-os-pr-uvisor-test-pipeline/796/console
CI Couldn't update status because of GitHub rate limit issue.

@mbed-bot
Copy link

Result: NOT_BUILT

Your command has finished executing! Here's what you wrote!

/morph test

@bridadan
Copy link
Contributor

Sorry for the NOT_BUILT error, this occurred because I enabled too much parallelization in the build pipeline and jobs were being dropped. Should be fixed now.

/morph test

@bridadan
Copy link
Contributor

Lots of CI issues today, sorry again. Had to restart the system. Restarting now.

/morph test

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1064

All builds and test passed!

@jamike
Copy link
Contributor Author

jamike commented Nov 16, 2016

any update ? Is there something blocking?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 16, 2016

@mazimkhan Failed to update commit status. err: java.io.IOException: API rate limit reached. Please rerun/check this

bridadan
bridadan reviewed Nov 21, 2016
View reviewed changes
platform/Callback.h
return _ops->call(this);
else return (R) 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

@geky Do you have any concern over these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch. The callback class should probably not be changed in the pr.

It is invalid to call a callback with no attached function. If this behaviour is needed, the callback can be checked at the call site like so:

Callback<void()> cb;
if (cb) {
    cb();
}

Or if the code relies on a default behaviour, the callback can be provided a noop function be default:

static void donothing() {}
Callback<void()> cb = callback(donothing);

if (something) {
    cb = callback(something_function);
}

cb();

This small function is actually very hot in mbed-os, and small changes may have surprising impacts. I would suggest a separate pr if this change is desirable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@geky Thanks for the explanation!

@jamike sorry for the delay and the late critique, but would mind taking a look at @geky's feedback?

bridadan
bridadan reviewed Nov 21, 2016
View reviewed changes
Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @jamike. I noticed one change to the Callback class and I wanted to get @geky's opinion on it. But other than that everything seems ok here.

@jamike jamike force-pushed the TARGET_STM_USBDEVICE_FS branch from 91ae6db to a34e093 Compare November 22, 2016 10:09
@jamike
Copy link
Contributor Author

jamike commented Nov 22, 2016

I fix the call back issue by modifying USBAudio and USBSerial as suggested in geky's comment.

@jamike
Copy link
Contributor Author

jamike commented Nov 24, 2016

Is something else blocking the merge ?

@adbridge
Copy link
Contributor

@geky please confirm you are happy with the changes

@adbridge
Copy link
Contributor

/morph test

@adbridge
Copy link
Contributor

@mbed-bot: TEST

HOST_OSES=ALL
BUILD_TOOLCHAINS=ALL
TARGETS=ALL

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 1153

All builds and test passed!

@adbridge
Copy link
Contributor

CI ran successfully.

@jamike
Copy link
Contributor Author

jamike commented Nov 25, 2016

@adbridge , geky already confirmed, he agreed 3 days ago, he reacted with thumb up in comment : I fix the call back issue by modifying USBAudio and USBSerial as suggested in geky's comment.

@geky
Copy link
Contributor

geky commented Nov 26, 2016

Yep, the change looks good to me, sorry if that wasn't clear.

@0xc0170 0xc0170 merged commit 93c08f3 into ARMmbed:master Nov 29, 2016
aisair pushed a commit to aisair/mbed that referenced this pull request Apr 30, 2024
Release 132 of the mbed library
12c3a2c 
Ports for Upcoming Targets

3241: Add support for FRDM-KW41 ARMmbed/mbed-os#3241
3291: Adding mbed enabled Maker board with NINA-B1 and EVA-M8Q ARMmbed/mbed-os#3291

Fixes and Changes

3062: TARGET_STM :USB device FS  ARMmbed/mbed-os#3062
3213: STM32: Refactor us_ticker.c + hal_tick.c files ARMmbed/mbed-os#3213
3288: Dev spi asynch l0l1 ARMmbed/mbed-os#3288
3289: Bug fix of initial value of interrupt edge in "gpio_irq_init" function. ARMmbed/mbed-os#3289
3302: STM32F4 AnalogIn - Clear VBATE and TSVREFE bits before configuring ADC channels ARMmbed/mbed-os#3302
3320: STM32 - Add ADC_VREF label ARMmbed/mbed-os#3320
3321: no HSE available by default for NUCLEO_L432KC ARMmbed/mbed-os#3321
3352: ublox eva nina - fix line endings ARMmbed/mbed-os#3352
3322: DISCO_L053C8 doesn't support LSE ARMmbed/mbed-os#3322
3345: STM32 - Remove TIM_IT_UPDATE flag in HAL_Suspend/ResumeTick functions ARMmbed/mbed-os#3345
3309: [NUC472/M453] Fix CI failed tests ARMmbed/mbed-os#3309
3157: [Silicon Labs] Adding support for EFR32MG1 wireless SoC ARMmbed/mbed-os#3157
3301: I2C - correct return values for write functions (docs) - part 1 ARMmbed/mbed-os#3301
3303: Fix #2956 #2939 #2957 #2959 #2960: Add HAL_DeInit function in gpio_irq destructor ARMmbed/mbed-os#3303
3304: STM32L476: no HSE is present in NUCLEO and DISCO boards ARMmbed/mbed-os#3304
3318: Register map changes for RevG ARMmbed/mbed-os#3318
3317: NUCLEO_F429ZI has integrated LSE ARMmbed/mbed-os#3317
3312: K64F: SPI Asynch API implementation ARMmbed/mbed-os#3312
3324: Dev i2c common code ARMmbed/mbed-os#3324
3369: Add CAN2 missing pins for connector CN12 ARMmbed/mbed-os#3369
3377: STM32 NUCLEO-L152RE Update system core clock to 32MHz ARMmbed/mbed-os#3377
3378: K66F: Enable LWIP feature ARMmbed/mbed-os#3378
3382: [MAX32620] Fixing serial readable function. ARMmbed/mbed-os#3382
3399: NUCLEO_F103RB - Add SERIAL_FC feature ARMmbed/mbed-os#3399
3409: STM32L1 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3409
3416: Renames i2c_api.c for STM32F1 targets to fix IAR exporter ARMmbed/mbed-os#3416
3348: Fix frequency function of CAN driver. ARMmbed/mbed-os#3348
3366: NUCLEO_F412ZG - Add new platform ARMmbed/mbed-os#3366
3379: STM32F0 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3379
3393: ISR register never re-evaluated in HAL_DMA_PollForTransfer for STM32F4 ARMmbed/mbed-os#3393
3408: STM32F7 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3408
3411: STM32L0 : map ST HAL assert into MBED assert ARMmbed/mbed-os#3411
3424: STM32F4 - FIX to add the update of hdma->State variable ARMmbed/mbed-os#3424
3427: Fix stm i2c slave ARMmbed/mbed-os#3427
3429: Fix stm i2c fix init ARMmbed/mbed-os#3429
3434: [NUC472/M453] Fix stuck in lp_ticker_init and other updates ARMmbed/mbed-os#3434
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geky geky geky left review comments

@0xc0170 0xc0170 0xc0170 left review comments

@bridadan bridadan bridadan left review comments

Assignees
No one assigned
Labels
release-version: 5.3.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jamike @sg- @0xc0170 @bridadan @mazimkhan @mbed-bot @adbridge @geky