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.

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

Uhuru RAVEN: Adding platform HAL #10115

Merged
merged 4 commits into from
Apr 4, 2019
Merged

Uhuru RAVEN: Adding platform HAL #10115

merged 4 commits into from
Apr 4, 2019

Conversation

junichikatsu
Copy link
Contributor

Description

Uhuru RAVEN: Adding platform HAL

This pull request is a reissue of the next pull request. I made a branch and made a pull request.
#9787

Test results:
GCC
ARM
IAR

Pull request type

[ ] Fix
[ ] Refactor
[x] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@ciarmcom ciarmcom requested review from screamerbg and a team March 15, 2019 06:00
@ciarmcom
Copy link
Member

@junichikatsu, thank you for your changes.
@screamerbg @ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-storage please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 15, 2019

travis tool jobs are failing with message : AssertionError: Target UHURU_RAVEN contains invalid device_name STM32F767VI . Please review

0xc0170
0xc0170 requested changes Mar 15, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Small changes request, looks fine to me otherwise

targets/TARGET_STM/TARGET_STM32F7/TARGET_STM32F767xI/TARGET_UHURU_RAVEN/uhuru_raven_init.h
* FUNCTION PROTOTYPES
* ----------------------------------------------------------------*/

void uhuru_raven_init(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to document this function here - what this function should do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the description of uhuru_raven_init function

targets/TARGET_STM/TARGET_STM32F7/TARGET_STM32F767xI/TARGET_UHURU_RAVEN/system_clock.c
@@ -0,0 +1,260 @@
/* mbed Microcontroller Library
* Copyright (c) 2006-2019 ARM Limited
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add SPDX identifiers for new files (PDX-License-Identifier: Apache-2.0) or BSD 3 clause for others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added SPDX identifier

@cmonr cmonr self-requested a review March 15, 2019 17:00
@junichikatsu
Copy link
Contributor Author

@0xc0170

travis tool jobs are failing with message : AssertionError: Target UHURU_RAVEN contains invalid device_name STM32F767VI . Please review

fixed and all travis tool jobs passed.

@0xc0170 0xc0170 requested a review from ashok-rao March 18, 2019 08:05
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 18, 2019

Thanks for the update, LGTM

@cmonr
Copy link
Contributor

cmonr commented Mar 18, 2019

CI started whilst reviews are completed

@mbed-ci
Copy link

mbed-ci commented Mar 18, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@cmonr
Copy link
Contributor

cmonr commented Mar 18, 2019

armclang: error: Failed to check out a license.

CI job restarted: jenkins-ci/mbed-os-ci_exporter

cmonr
cmonr reviewed Mar 19, 2019
View reviewed changes
targets/TARGET_STM/TARGET_STM32F7/TARGET_STM32F767xI/TARGET_UHURU_RAVEN/system_clock.c
#include "stm32f7xx.h"
#include "nvic_addr.h"
#include "mbed_error.h"
#if TARGET_UHURU_RAVEN
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but why is this needed if this file is inside of its own TARGET subdir?

cmonr
cmonr approved these changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 20, 2019

@ashok-rao please review

@cmonr
Copy link
Contributor

cmonr commented Mar 27, 2019

@junichikatsu

#9787 (comment)

@junichikatsu
Copy link
Contributor Author

@cmonr
I'm sorry.
I'll go back with the commit and then rebase.

@cmonr
Copy link
Contributor

cmonr commented Mar 27, 2019

@junichikatsu Thank you! I know it takes a bit of getting use to (I recall having a hard time with it when I started), but there are decent technical reasons why rebases are prefered in this repo.

@adbridge
Copy link
Contributor

@screamerbg @ARMmbed/mbed-os-storage could we get a review on this please?

davidsaada
davidsaada approved these changes Mar 27, 2019
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

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

LGTM, other than the redundant TARGET_UHURU_RAVEN ifdef, (addressed by @cmonr's), which is not a blocker anyway.

@toyowata
Copy link
Contributor

toyowata commented Apr 3, 2019

@adbridge Hi. I believe @screamerbg is on holiday this week. Can you assign someone in TAM if you need more reviewer? e.g. @ashok-rao or @MarceloSalazar

ashok-rao
ashok-rao reviewed Apr 3, 2019
View reviewed changes
targets/targets.json
"config": {
"clock_source": {
"help": "Mask value : USE_PLL_HSE_EXTC | USE_PLL_HSE_XTAL | USE_PLL_HSI",
"value": "USE_PLL_HSE_XTAL",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the XTAL frequency used here?

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 used 8MHz XTAL.

@ashok-rao
Copy link
Contributor

Small comment, otherwise looks good to me.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Apr 3, 2019

Test run: FAILED

Summary: 1 of 1 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 3, 2019

CI restarted (internal error)

@mbed-ci
Copy link

mbed-ci commented Apr 3, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 merged commit 6081727 into ARMmbed:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashok-rao ashok-rao ashok-rao left review comments

@cmonr cmonr cmonr approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@davidsaada davidsaada davidsaada approved these changes

@screamerbg screamerbg Awaiting requested review from screamerbg

Assignees
No one assigned
Labels
release-version: 5.12.1 scope: new-target
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@junichikatsu @ciarmcom @0xc0170 @cmonr @mbed-ci @adbridge @toyowata @ashok-rao @davidsaada