search

ARMmbed / nrf51-sdk

Module to contain files provided by the nordic nRF51 SDK
Other
13 stars 17 forks source link

Add all files under drivers_nrf folder to this module #5

Closed LiyouZhou closed 8 years ago

LiyouZhou commented 8 years ago

@rgrover @ARMmbed/ble-owners @marcuschangarm I have added all files under drivers_nrf to this module. they don't build properly. My solution is to yotta_ignore all unused files.

But I feel it is pointless to add files you don't actually use. Because they don't even compile properly without a lot of messing around

If anybody want to use some files, they are welcome to add the files they actually need with the modifications to make them work with yotta to a pull request.

Please tell me I'm wrong.

rgrover commented 8 years ago

@LiyouZhou so these are files from the original SDK which aren't necessary, but available to be included. Is that correct?

marcuschangarm commented 8 years ago

I don't like defining what is necessary and what is not. Our users might have different use-cases than us. Literally all the other platform partners have full driver support for their chip families in mbed OS beyond what is being used by the mbed HAL.

What build errors are you seeing exactly?

LiyouZhou commented 8 years ago

@marcuschangarm The problems are:

  1. Folder structure of Nordic sdk does not conform to the yotta module design. We need mass use of extraIncludes.
  2. They provide stuff in the sdk that we have replaced with our stuff such as timer.
  3. A lot of definitions are missing. They reply on defs being passed on command line which we do not have.
  4. Use of language beyond c99 which we do not support.
  5. Duplicate filenames that does completely different things.
  6. They provide a large number of files (1416 in components), if all compiled, takes long time. Takes a long time to download as well.

If nordic decided to mangle their sdk and make all the modification to fit into mbed like all the other vendors do, they are encouraged. But currently the sdk provides a lot more than what is suitable for mbed, and does not conform to our build system.

rgrover commented 8 years ago

this PR is not to be merged. This is just a reminder that we may not want to include all of the original SDK.

marcuschangarm commented 8 years ago

I see your point. How does it look like if you only add modules that doesn't have an mbed OS equivalent? What about only the header files?

I've had the need for non-mbed components a couple of times, sometimes I need both the header file and implementation. Other times the header file is enough.

LiyouZhou commented 8 years ago

@marcuschangarm Your comment prove eactly my point. Everyone's need is different, it would be difficult for me to anticipat the future needs and add files. It would be better if the person who need the files would add the files.

marcuschangarm commented 8 years ago

You are assuming that the users know they have the option of adding files and know where and how to get them.

People might just abandon the platform if there is no clear way to use the on-chip modules.

LiyouZhou commented 8 years ago

I imagine most user does not want to get so low level stuff. But if they do, there are two ways:

  1. Add files following instructions in Readme.md
  2. Add all files in SDK and yotta ignore most of them. User can unignore as needed

Before doing that. I want to make sure we think through why a user would want to access features not supported by mbed and whether we want to support them doing that. If user can access the full power of Nordic SDK, what is nordic's view on this.

Why are you using Nordic specific files anyways? Other chips are coming out rapidly, your code wouldn't have the compatibility mbed promised to have.

There is also the issue of tracking the changes we made on top of Nordic files. I can see if we don't control the repo, it can get out of hand quickly. On 2 Dec 2015 2:15 p.m., "Marcus Chang" notifications@github.com wrote:

You are assuming that the users know they have the option of adding files and know where and how to get them.

People might just abandon the platform if there is no clear way to use the on-chip modules.

— Reply to this email directly or view it on GitHub https://github.com/ARMmbed/nrf51-sdk/pull/5#issuecomment-161309151.

marcuschangarm commented 8 years ago

Why are you using Nordic specific files anyways?

Thats a discussion best to have for offline... :p

rgrover commented 8 years ago

eventually, this module will be maintained by Nordic. Let's ensure that we start a dialogue with Nordic to offload this responsibility