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

New target future sequana #8491

Merged
merged 9 commits into from
Nov 3, 2018

Conversation

lrusinowicz
Copy link
Contributor

Description

@MarceloSalazar

This commit adds support for Future Electronics Sequana target (FUTURE_SEQUANA) including basic support for Cypress PSoC 6 microcontrollers.
Tests were run on a Sequana board and are all passing for all 3 compilers (GCC_ARM, ARM, and IAR).

tests_sequana_arm_20181020.log
tests_sequana_gcc_20181002.log
tests_sequana_iar_20181020.log

Pull request type

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

Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

I'm not convinced that the image merging present in this PR is something we should take. My concerns are:

  • Infrastructure is added to the exporters to support a probably-broken Makefile export.
  • This will break managed bootloader mode if you ever decide to turn that on.
  • I don't know how a user would add a new M0 firmware without modifying Mbed OS.

# "classname.functionname"
temp = hook_data["export_function"].split(".")
if len(temp) != 2:
raise HookError(
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of HookError before its definition.


if class_name not in mdata or \
not inspect.isclass(mdata[class_name]):
print (mdata)
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug print left in.

not toolchain_labels.intersection(toolchain_restrictions):
return None
# Finally, get the requested function
print("Found function '%s' in class '%s'."% (function_name, class_name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Dedub print left in.

Positional Arguments:
target - the target object for inspection
toolchain - the toolchain object for inspection
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks like it's copied from https://github.com/ARMmbed/mbed-os/blob/master/tools/targets/__init__.py#L330-L380. Please don't copy code.

@@ -158,6 +216,9 @@ def generate_project_files(resources, export_path, target, name, toolchain, ide,
exporter_cls, _ = get_exporter_toolchain(ide)
exporter = exporter_cls(target, export_path, name, toolchain,
extra_symbols=macros, resources=resources)
# Locate target-specyfic exporter if specified.
exporter.TARGET_EXPORTER = get_target_exporter(target, toolchain)
Copy link
Contributor

Choose a reason for hiding this comment

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

Setting a class-constant is generally not a good idea. This should be passed into the constructor, or at least an object member.


# update metadata
sig_bytes = ihex.tobinarray(start=0x90500002, size=4)
signature = (sig_bytes[0] << 24) | (sig_bytes[1] << 16) | (sig_bytes[2] << 8) | sig_bytes[3]
Copy link
Contributor

Choose a reason for hiding this comment

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

Would struct.unpack work better here?

for feature in image_features:
if not (feature in target_features):
return False
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

This function could also be written as:

return all(feature in target_features for feature in image_features)

for image in m0_images:
features = image['features']
if check_matching_features(image['features'], target_features):
for hexf in resources.hex_files:
Copy link
Contributor

@theotherjimmy theotherjimmy Oct 22, 2018

Choose a reason for hiding this comment

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

resources.hex_files is the names of all of the hex files in the build. What you want is the paths for every hex file in the build. This is available as:

from tools.resources import FileType
resources.get_file_paths(FileType.HEX)

The current code will fail in the online compiler.

break

if m0hexf:
toolchain.notify.info("M0 core image file found %s." % os.path.basename(m0hexf))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make this a toolchain.notify.debug.

tools/toolchains/__init__.py Show resolved Hide resolved
@@ -23,11 +23,13 @@ namespace mbed {

Timer::Timer() : _running(), _start(), _time(), _ticker_data(get_us_ticker_data()), _lock_deepsleep(true)
{
(void)ticker_read_us(_ticker_data); // Make sure h/w timer is initialized in non-critical context.
Copy link
Contributor

Choose a reason for hiding this comment

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

This fix should be at least a separate pull request. best if it's a separate pull request to have it reviewed separately from adding new target.

Choose a reason for hiding this comment

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

Indeed, this is something that affects multiple platforms, so the change shouldn't be introduced on this PR.
@lrusinowicz please remove this and send a separate PR if really needed. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created new pull request for this fix: #8502
Please make sure that it is merged before this one ( #8491 ).


#undef RPC_GEN

extern "C" {
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these functions defined with C linkage 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.

I have done it for consistency, because functions that are RPC-ed to M0 (from psoc6_utils.h) are low level ones, called from HAL implementation, so they are necessarily C API, I also wanted this mechanism to be most generic, so it meant lower common denominator, i.e. C. The third reason is that wrapper generation is a code shared between M0 and M4 cores, and for pre-compiled binaries for M0 core we are providing we have to use PSoC Creator IDE from Cypress and it is C-only.
However, you are right the the wrapper functions themselves are compiled with CPP compiler on Mbed, so only wrappers APIs have to be declared as C-linkage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood.

However, you are right the the wrapper functions themselves are compiled with CPP compiler on Mbed, so only wrappers APIs have to be declared as C-linkage.

Fix to apply to only wrappers API? I did not notice which ones are.

* @param obj The analogout object
* @return A floating-point value representing the current voltage on the pin,
* measured as a percentage
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we having doxygen comments here (copy paste from header file, if we improve documentation in the analogout header file, this might be outdated and hard to maintain if each target implementation does it).

This is valid for any HAL implementation ( I can see it in every _api HAL code file).

* @param[in] obj The SPI peripheral to check
* @return non-zero if the peripheral is currently transmitting
*/
int spi_busy(spi_t *obj) {
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 run astyle on the HAL implementation ({ should be on the new line). Currently astyle check ignores target folder but will be enabled in the next release most likely.

int i2c_stop(i2c_t *obj_in)
{
i2c_obj_t *obj = OBJ_P(obj_in);
// Cy_SCB_I2C_MasterSendStop(obj->base, obj->timeout, &obj->context);
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, please remove

/** Send START command
*
* This function is a dummy operation on PSoC
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, having this is OK as it is different from i2c_start documentation and explains why this is empty (regarding my earlier comment about having copy-paste API docs in the implementation)

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2018

@yanesca @hanno-arm Please review trng implementation

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2018

Depends on #8502 (label preceding PR set)

@yanesca
Copy link
Contributor

yanesca commented Oct 23, 2018

@0xc0170 I'll have a look at it!

After a superficial look I already noticed that this contribution contains crypto accelerator headers and binaries as well. I also noticed that they are not integrated with Mbed TLS, therefore applications using Mbed TLS interface won't make use of them. I just would like to make sure that we all are aware of this and are happy to go forward without the integration.

@0xc0170 @lrusinowicz Can you both confirm?

@lrusinowicz
Copy link
Contributor Author

@theotherjimmy
I'll start from the end, as the answer is a simplest one.

I don't know how a user would add a new M0 firmware without modifying Mbed OS.

Hex files are detected (and added to the resource list) anywhere in the [source] tree. So if one wants to use it's own image for M0 it can place it in e.g the program root directory or any subdirectories of it, excluding BUILD.
I've also added functionality to merging (hook) functions to look for hex into the parallel target BUILD directory (if "enable_dual_core_build" switch is turned on). This assumed, that user application intended to utilize both cores would have target sub-directories (TARGET_MCU_PSOC6_M0 and TARGET_MCU_PSOC6_M4 or TARGET_FUTURE_SEQUANA_M0/TARGET_FUTURE_SEQUANA) holding source code unique to particular core.
The build process would then create two binary trees, BUILD/FUTURE_SEQUANA and BUILD/FUTURE_SEQUANA_M0 and will look for a hex file into the latter when merging images during FUTURE_SEQUANA build.

This will break managed bootloader mode if you ever decide to turn that on.

Certainly, bootloader functionality sound great and most likely we would to have it enabled some time in the future. This, however, will require resolving some strategy issues, as the PSoC 6 is unique in Mbed with it's dual core hardware, so this won't happen automagically. Likely, bootloader will have to be implemented on M0 core, and right now M0 is not the main target of Mbed OS for this platform.

Infrastructure is added to the exporters to support a probably-broken Makefile export.

I've once had changes that allow user to build complete project for both cores with the assumptions mentioned above, and it . This, however, was previously rejected/vetoed as too complicated/cumbersome, possibly jeopardizing ARM plans to cover multicore targets.
Maybe in this situation I should resort to just minimal implementation, having user to manually select an image for M0. This strategy would probably result in removing most of the offending code. The downsides would be:

  • User will have to manually select different M0 binary for BLE applications by means of mbed_app.json. Probably not and issue as one already has to add BLE feature using the same file.
  • Users willing to create dual-core applications would have to copy over binary result of M0 compilation out of the BUILD directory and, possibly, rename it (for consistency).

Does it sound as a better plan?

@theotherjimmy
Copy link
Contributor

So if one wants to use it's own image for M0 it can place it in e.g the program root directory or any subdirectories of it, excluding BUILD.

I disagree. The post-build script looks into the array: https://github.com/ARMmbed/mbed-os/blob/16ee057d462219ac9b7069fde1021c06946119df/targets/targets.json#L4550-L4561 and picks one of those images if it's present: https://github.com/ARMmbed/mbed-os/blob/16ee057d462219ac9b7069fde1021c06946119df/tools/targets/PSOC6.py#L144-L152 This makes the built-in images the only options as the post-build script raises a ConfigException if it can't find a built-in image, and it's a crap shoot if you name your file the same as a built in image: https://github.com/ARMmbed/mbed-os/blob/16ee057d462219ac9b7069fde1021c06946119df/tools/targets/PSOC6.py#L154-L158 My understanding of the code is that you can't provide a new image to the post-build script without enabling this sub-target-build-enabled configuration parameter and building the image yourself https://github.com/ARMmbed/mbed-os/blob/16ee057d462219ac9b7069fde1021c06946119df/tools/targets/PSOC6.py#L120-L128 which does not work with custom build directories specified with --build.

A big issue I have with this post-build script is that it seems to be targeting a use case that is very similar to managed bootloader mode. Adding a second, target-specific merging functionality is a disadvantage for us in the long run as we would have an inconsistent story. Our story would be something like "use managed bootloader mode configuration parameters, unless your using a PSoC6, then use this other set of parameters that do nothing on every other target. Oh and further, your manager bootloader configuration will break the PSoC6". It's much easier for us to document and developers to use if the story is "use managed bootloader mode, it works on every target that can bootloader". My preference in this situation is to add a minimal target implementation and work or creating configuration to describe the nuance that your target adds to managed bootloader mode. I suspect that your target will not be alone in it's nuance, and that other targets will be able to take advantage of new features we add.

as the PSoC 6 is unique in Mbed with it's dual core hardware, so this won't happen automagically. Likely, bootloader will have to be implemented on M0 core, and right now M0 is not the main target of Mbed OS for this platform.

The bootloader mode has no requirement that the image be built for the same target. The only requirements are that the image specified to target.bootloader_img starts the application that you're compiling now, and looks at the flash erase block after it's end for the vector table of the current application.

Does it sound as a better plan?

Yes. We should have an offline discussion about what's special about your target and how we can add some general-purpose tools features that make it easier for developers to take advantage of these special features. For reference, the tools and post build scripts have to work with older versions of Mbed OS, meaning that any new features you add will have to be compatible with this post-build script.


Supposing that you can enable bootloader functionality now, we can alleviate the first downside you mention by:

  • Configuring the "just bootloder" M0 image as the bootloader in targets.json.
  • Adding an mbed_lib.json within a FEATURE_BLE directory that overrides this configuration with the BLE bootloader + stack
  • Allowing the user to specify their own target.bootloader_img in their mbed_app.json

This would at least remove the need to configure the bootloader, and retain the ability to use the bootloader of their choosing.

@lrusinowicz
Copy link
Contributor Author

I disagree. The post-build script looks into the array:

One would just need to overload array definitions by means of mbed_app.json. I haven't mention this, as it will always be the case.

The other thing is, that the code running on M0 is not a bootloader. Not only it's different functionally, but right now it doesn't even provide bootloading capabilities. Memory allocation schema is also different. And so on... So calling it a bootloader will just confuse developers.
Unfortunately, adding a bootloader capabilities to SEQUANA/PSOC6 target at this moment is not part of the plan.

Also, there are many targets having special post binary handling (at least all those having hook classes in tools/targets/_init_.py) or unusual parameters. I understand, that you are opposing creation of yet another special case, so I'm working on reducing it to the bare minimum. Hopefully, we can develop better (more consistent) way to handle this further down the road, maybe even fixing exporters as a side benefit. :)

@lrusinowicz
Copy link
Contributor Author

@yanesca

I also noticed that they are not integrated with Mbed TLS, therefore applications using Mbed TLS interface won't make use of them.

My understanding is that it's exactly the case. TLS is a different use case, on which Mbed TLS developers are working right now. I can remove TRNG implementation if you believe it will be troubling; we are not using it right now.

@yanesca
Copy link
Contributor

yanesca commented Oct 25, 2018

@lrusinowicz I don't think that it is troubling I just wanted to make sure that this is what you want.

I wanted to make it sure because TRNGs are handled differently from crypto accelerators in Mbed OS:

  • TRNGs are integrated in HAL
  • Crypto accelerators are integrated in Mbed TLS (which is not just a TLS stack, but a cryptographic library as well and most applications use it as a uniform interface to hardware accelerators instead of the drivers directly).

@theotherjimmy
Copy link
Contributor

Not only it's different functionally, but right now it doesn't even provide bootloading capabilities.

Managed bootloader mode is really just a way to:

  • Reserve space for an application than resides before the current application in ROM,
  • Merge the bootloader and application as part of a post build step,
  • In a target-independent way.

Even if you don't call the M0 code a bootloader, it seems like these steps would be useful for developers using your target. These steps should be especially useful as they understand flash erase blocks and will round up/down to the nearest one as needed. If we can generalize parts of your target's nuances, it may be possible for other targets to take advantage of these new features. For example, reserving RAM for the M0 could use the same mechanism as reserving RAM for a Nordic SoftDevice or a v8m secure partition.

Memory allocation schema is also different.

Please elaborate. That would be useful information.

So calling it a bootloader will just confuse developers.

Sure, I understand that unexpected terms can be confusing. What would you call it? a "SoftDevice" like Nordic? a "secure partition" like v8m?

We can alias the managed bootloader mode configuration so that it matches the terms you expect or better suite the use case.

I understand, that you are opposing creation of yet another special case, so I'm working on reducing it to the bare minimum.

Reducing the special cases now will help get it in faster, as it will remove this discussion from the list of blockers. We certainly should work to maximize the support of your target, and I think that improving support can come as follow on PRs.

Hopefully, we can develop better (more consistent) way to handle this further down the road, maybe even fixing exporters as a side benefit. :)

Agreed! :) We should develop a more consistent way to handle this, and I'm willing to support extra exporter infrastructure if it comes with consistent support across all targets. I'll even collaborate on the tooling changes.

@lrusinowicz
Copy link
Contributor Author

I believe I've addressed all the issues with the latest set of commits.

@0xc0170 you can remove the PR dependency tag. I have changed our implementation of us_ticker and resource manager a bit to have all the resources for us_ticker pre-reserved. This way us_ticker_init() is safe to be called from critical section now and we no longer need Timer changes.

I'm attaching test results for the latest version.
tests_sequana_arm_20181027.log
tests_sequana_gcc_20181027.log
tests_sequana_iar_20181027.log

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.

Files libcy_crypto_client_mdk.a and similar also require license to be included as mentioned earlier - still cant find it here and what theyactually provide (simple readme would be helpful).

* This file is automatically generated by PSoC Creator.
*
********************************************************************************
* Copyright (c) 2007-2017 Cypress Semiconductor. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed all files have this copyright notice, but missing license file ? I could not locate any license file in folders. What is the license for these driver files ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added README and a license file (copied verbose from Cypress PDL). Is it ok now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, Readme looks good to me. we will review the license

I noticed also some libs precompiled (ble or related), that does not have any license (the latest update covers hex/ folder ones. Please review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are also parts of the Cypress PDL. I've added more readme and license files to cover.

@hanno-becker
Copy link

@0xc0170 Could you remove the review request from me? @yanesca has taken over the crypto review work again.

@0xc0170 0xc0170 removed the request for review from hanno-becker October 30, 2018 13:55
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

This is much simpler, and directly compatible with managed bootloader mode when target.sub_target is not set. While I'm not convinced that target.sub_target is the correct name, it's optional and not worth blocking on.

targets/targets.json Outdated Show resolved Hide resolved
@MarceloSalazar
Copy link

@0xc0170 can you remove the TLS label, please?

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

Build : FAILURE

Build number : 3532
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8491/

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

Build : FAILURE

Build number : 3533
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8491/

@adbridge
Copy link
Contributor

adbridge commented Nov 2, 2018

Something strange going on in CI with this, trying again.

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

Build : SUCCESS

Build number : 3534
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8491/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz There's a lot of noise in the results because it looks like we had some exporter node issues, but please take a look at the following logs:

All of them failed with the message file contains no data. Suspecting something is wrong with how the new target is interacting with the makefiles that make up these compilers.

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz I was reminded that the command that shows up in the log involves some steps that are prepared by the CI...

If the quickest thing for you is to simply disable the file exporting, which can be confirmed by running the mbed export -S command, and verifying that FUTURE_SEQUANA and FUTURE_SEQUANA_M0 don't have any make_x checked.

@lrusinowicz
Copy link
Contributor Author

It looks like indeed, the srec_cat has problem interpreting hex files. But this result I only get when I run srec_cat without properly specifying file format with -intel option. When I run it with a command line optione as specified in a makefile it works properly.
Which version of srec_cat utility do you have installed on the servers?

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz What python module package is this a part of? I've never head of the utility before.

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz Nevermind. Found the utility you were refering to.
Would you mind providing the version you're using to develop with? I can ask someone who has access to see what version we have.

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz Fwiw, I compiled a recent version for my work VM (Arch Linux), and found that make (after running mbed export -i make_gcc_arm -m FUTURE_SEQUANA) still failed.

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

@lrusinowicz When you're ready, I can restart CI with the updated commit.

Already locally verified that the three failed exporters should be disabled.

@lrusinowicz
Copy link
Contributor Author

@cmonr
Yes, I was also able to reproduce the issue locally. I've already submitted a patch disabling makefile exporters.

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

@lrusinowicz
Copy link
Contributor Author

@cmonr
I've verified that the only exporters reported as enabled for SEQUANA targets are codeblocks, e2studio and gnuarmeclipse.
Codeblocks exporter works, as I'm using it daily.
Running gnuarmeclipse doesn't report errors, but the generated project doesn't seem to be correct - eclipse doesn't complain, but the project seems to be empty.
I have no idea about the e2studio.

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

Build : SUCCESS

Build number : 3535
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8491/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Nov 2, 2018

Test : SUCCESS

Build number : 3320
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8491/3320

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

/morph mbed2-build

@cmonr
Copy link
Contributor

cmonr commented Nov 2, 2018

I was starting to get concerned since the exporter CI tests generally take no more than two hours when started, but was relieved to see that the job is just being starved of Windows Nodes due to the autotriggering of other export jobs.

The jobs that have completed thus far are all green. Unable to provide an eta atm, but holding 5.10.3 PR generation for this PR.

@mbed-ci
Copy link

mbed-ci commented Nov 3, 2018

Exporter Build : SUCCESS

Build number : 3147
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8491/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.