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

9160 Socket Network Support #2517

Open
wants to merge 8 commits into
base: develop-nordic-zephyr-port
Choose a base branch
from

Conversation

dboling-ericsson
Copy link

Description

Adds non-secure network support for nrf9160.

Motivation and Context

Feature add

How Has This Been Tested?

Limited testing with HttpSample

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

Adds non-secure network support for nrf9160
nfbot and others added 2 commits January 27, 2023 01:00
Automated fixes for code style.
…176-ad21-4553-9e82-ebf345d34981

Code style fixes for nanoframework/nf-interpreter PR#2517
Copy link
Author

@dboling-ericsson dboling-ericsson left a comment

Choose a reason for hiding this comment

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

Style changes look good.

dboling-ericsson and others added 3 commits January 27, 2023 11:24
fixes build problem due to NATIVE_PROFILE_PAL_NETWORK() being defined outside commit
Automated fixes for code style.
Copy link
Member

@Ellerbach Ellerbach 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 the addition, that will be useful! Few comments more on the style. Please don't forget to merge the latest code style PR into your branch. In general, we don't keep dead code or we document why it's there. And we don't keep the traces. Also, in some of the code, it would be great to have few more comments to understand what's done and for some of the key functions a good description of what they are doing. In general, that does help a lot for the maintenance.

@@ -153,7 +153,7 @@ macro(nf_setup_target_build)

zephyr_compile_definitions(PROJECT_NAME=nanoCLR)

zephyr_compile_definitions(APP_VERSION=${BUILD_VERSION})
#zephyr_compile_definitions(APP_VERSION=${BUILD_VERSION})
Copy link
Member

Choose a reason for hiding this comment

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

if you comment this line of code, please add a little comment on why :-)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry for the sloppy code. I wanted to post what I could as I'm not sure when I'll be able to work on the code again. I'll try to clean things up today.

targets/Nordic/_include/sockets_nrf9160.h Show resolved Hide resolved
targets/Nordic/_nanoCLR/targetHAL_Time.cpp Show resolved Hide resolved
targets/Nordic/_nanoCLR/targetHAL_Time.cpp Show resolved Hide resolved
…dbb-b28d-461e-94f4-1a829a1ecf42

Code style fixes for nanoframework/nf-interpreter PR#2517
@dboling-ericsson
Copy link
Author

The 'build failing' bug is strange. I always build the code before uploading. I'm trying now, to build and have the build pull down the NCS instead of referencing a local copy. If that fails, I'll know what the issue is.

@dboling-ericsson
Copy link
Author

I pushed up a fix to the build error. It was a problem with the ARDESCO target not having the proper configuration. In my rush, I hadn't built that version before posting. Fixed and tested all 3 targets.

@josesimoes
Copy link
Member

I pushed up a fix to the build error. It was a problem with the ARDESCO target not having the proper configuration. In my rush, I hadn't built that version before posting. Fixed and tested all 3 targets.

Awesome! Thanks for fixing it.

@josesimoes
Copy link
Member

@dboling-ericsson I've started reviewing this.
It's a lot of code and I wanted to test it thoroughly.
Just wanted to give you a note that this is not forgotten and it's highly appreciated. 😃

@dboling-ericsson
Copy link
Author

Great to hear. I don't have as much time for this now but am happy to answer questions and to fix something if I can.

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.

4 participants