-
Notifications
You must be signed in to change notification settings - Fork 3k
Add support for the WISE1530 and integration framework for WICED SDK #5758
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
Conversation
@@ -0,0 +1,208 @@ | |||
#include "mbed_assert.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
license header here please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is updated
|
||
/* notify link is up or down | ||
*/ | ||
wiced_result_t wiced_network_notify_link_up(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
new line
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
#if defined (IAR_TOOLCHAIN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use IAR identifier rather here
why a header file inclusion is based on the toolchain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file had a lot of revisions, I'll see if I can remove it when I can.
* limitations under the License. | ||
*/ | ||
#pragma once | ||
#if 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is external file and imported as is (so this deadcode is in there) ? same for typedef below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good catch, will remove
#include "wwd_assert.h" | ||
#include "wwd_rtos_interface.h" | ||
#include "wiced_utilities.h" | ||
#include "mbed.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not include this , pull in what is needed rather
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above comment got hidden by changes, but question still stands:
Is this a problem in cpp files? There's no risk of namespace polution since it's self contained in the cpp file. If anything this would help protect against filename changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Including only header files that you need will greatly reduce the rebuild time.
On the first build, dependency files are created. Then on the following builds, if there has not been any changes to those headers that you include, your file is not needed to rebuild.
But if everyone includes "mbed.h" this leads every small change to lead to full rebuild of the whole Mbed OS tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, will update
#include "ticker_api.h" | ||
|
||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these docs needed here? Not done in the header file? If they are, this is duplicate and pollutes code file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're copied from the header file, but the header file is hard to track down. I can remove them if we'd like.
|
||
#include "WISE1530Interface.h" | ||
|
||
#include "mbed.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont include this header file outside of user space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, is this a problem in cpp files? There's no risk of namespace polution since it's self contained in the cpp file. If anything this would help protect against filename changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, a name.cpp
file should only ever include its name.h
file, for stylistic reasons, not technical.
@0xc0170 Could you re-review please? |
Please review travis, docs and tools report related errors. |
I may need help, @theotherjimmy:
What are valid assembly names?
This means I don't have a pack for the target I think? I'm not entirely sure what this response to this assertion should be. |
Ah, sorry @AnotherButler I should have clarified. The WICED.md file is probably the only document worth editing: The code you're editing predates me, I don't know if it's valuable to edit and I can't vouch that the edits are keeping the documentation's meaning correct, sorry. |
@geky Thanks for the clarification. |
|
Thanks, will update the assembly file. I'll have to look into the cmsis pack thing more. Where does one normally find cmsis packs? Do they need to be generated? |
Yes, a vendor generates a pack and publishes it (device support as it is called there). You can use the arm pack manager that is part of this repo, or check in the uVision for instance (it has the pack installer to check if target is there). If there is not, we shall remove device_name. @theotherjimmy can provide more info in this area |
Should we rename folder mbed-os\targets\TARGET_WICED\TARGET_WISE1530_F412RE\ and files under that folder? Those files are not hw specifig files just for wise1530, they are more like wiced adaptation to mbed and can be used with other variants too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my comments to WICED.mk
targets/TARGET_WICED/WICED.md
Outdated
- mbed_lib.json | ||
|
||
## Download the WICED SDK | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are two different instructions, so I would add comment:
"Both WICED SDK and patch from Advantech is needed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to section header
targets/TARGET_WICED/WICED.md
Outdated
on Advantech's Wiki: | ||
http://ess-wiki.advantech.com.tw/view/WISE-1530_SDK | ||
|
||
To get the WICED SDK, download WICED-Studio-5.2.0 for your platform: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or WICED SDK WICED-Studio-5.2.0 for your platform can be get here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KariHaapalehto, I'm not exactly sure what the suggestion is for this line. Does the studio need to be named "WICED SDK WICED-Studio-5.2.0"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's not right, the studio is WICED-Studio-5.2.0. Inside the studio you get the WICED SDK.
targets/TARGET_WICED/WICED.md
Outdated
To get the WICED SDK, download WICED-Studio-5.2.0 for your platform: | ||
https://community.cypress.com/community/wiced-wifi/wiced-wifi-documentation | ||
|
||
Once downloaded, move the WICED SDK folder into `targets/TARGET_WICED`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once downloaded, move the SDK's targets/TARGET_WICED/43xxx_Wi-Fi-folder into targets/TARGET_WICED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also added note about rename to WICED
targets/TARGET_WICED/WICED.md
Outdated
For the WISE1530, download the patch from Advantech: | ||
`WM-BN-BM-22_SDK_5.1.x_platform_patch.zip` | ||
|
||
Once downloaded, copy the contents into `targets/TARGET_WICED/WICED`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once downloaded, copy the contents into targets/TARGET_WICED
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It really should be targets/TARGET_WICED/WICED
(after rename). Otherwise the libraries/platforms folders don't line up with their destinations. Note there are multiple nested WICED folders.
targets/TARGET_WICED/WICED.md
Outdated
|
||
1. Remove these files because they are unnecessary and large: | ||
- `targets/TARGET_WICED/WICED/tools` | ||
- `targets/TARGET_WICED/WICED/build` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't build-directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build directory is generated if you compile using the WICED studio. I can go ahead and remove this if we want.
Note this is required for the WISE1530 but may not be needed for other | ||
platforms. | ||
|
||
1. Remove the includes of `w_core_cm4.h` and `w_core_cmFunc.h`, and replace the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ #include "core_cm4.h" | ||
``` | ||
|
||
1. Remove the padding of SDPCM data packets. This requires modifications in two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} sdpcm_data_header_t; | ||
``` | ||
|
||
In `targets/TARGET_WICED/WICED/WWD/internal/w_wwd_sdpcm.c` line 1282: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 1441:
Note this is required for the WISE1530 but may not be needed for other | ||
platforms. | ||
|
||
1. Set up SDIO_ENUMERATION_TIMEOUT_MS to be configurable. Mbed OS will override this, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12
#define SDIO_ENUMERATION_TIMEOUT_MS (500) | ||
+ #endif | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 12 Rename ./mbed-os/targets/TARGET_WICED/TARGET_WISE1530_F412RE/wise1530_emac.c to ./mbed-os/targets/TARGET_WICED/TARGET_WISE1530_F412RE/wise1530_emac.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? wise1530_emac.c is in Mbed OS, so we can change it here if need be.
But why does it need to be changed to cpp, was there a compilation issue?
As this is only going for the feature branch and is a work in progress, I'm OK if this gets merged in as it is currently. We will anyway continue on working to improve the instructions but at least we won't be blocking others to study it. |
acc7539
to
5e135e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one comment
#include <stdio.h> | ||
#include "CordioBLE.h" | ||
#include "CordioHCIDriver.h" | ||
#include "mbed.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is needed here from mbed.h ? DigitalInout and PinNames ?
missing license header file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like it was not used
d3f9aef
to
516fd7b
Compare
@0xc0170 @SeppoTakalo @KariHaapalehto Are you all happy with @geky's updates? |
I'm still in a process of going though the steps... I have not yet successfully build it. |
Final problem that I cannot get past
This |
Also, lots of files seems to be relying that |
Looks like it's almost good to go. @SeppoTakalo thanks for fixing that 👍 |
Test : FAILUREBuild number : 847 |
I'll restart it. @geky, the only thing that I could really think of is if the CI machine running the test is being hit hard, and having issues keeping up with all of the communication with the Device Under Test. /morph uvisor-test |
/morph test |
Sorry, but it looks like this PR is doing something to lp_ticker test with the NUCLEO_F746ZG. The device is working fine, as evidenced by its previous build (#5022 (comment)). Results are about to be reported. |
Test : FAILUREBuild number : 851 |
Please rebase and resubmit - hopefully the cause of the failures should have been cleared on master. |
This is a squash of a long series of work that introduces the WICED SDK into Mbed OS. Allowing the use of the WISE1530Interface in Mbed OS. The WICED SDK requires additional steps to integrate into Mbed OS before it can be used. For more information see WICED.md.
Copy edit for active voice, grammar and precise language.
/morph build |
Build : SUCCESSBuild number : 1094 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 772 |
Test : FAILUREBuild number : 902 |
Test : SUCCESSBuild number : 903 |
🎉 |
Add support for the WISE1530 and integration framework for WICED SDK
This is a squash of a long series of work that introduces the WICED SDK into Mbed OS. Allowing the use of the WISE1530Interface in Mbed OS.
The WICED SDK requires additional steps to integrate into Mbed OS before it can be used. For more information see WICED.md.
cc @pan-, @SeppoTakalo, @SenRamakri, @sg-, @AnotherButler