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

Add nrf5 platform #830

Merged
merged 2 commits into from May 14, 2017
Merged

Add nrf5 platform #830

merged 2 commits into from May 14, 2017

Conversation

d00616
Copy link
Contributor

@d00616 d00616 commented Apr 23, 2017

This is the support of the Nordic Semiconductor NRF51 (nrf51822, nrf51422) and NRF52 (nrf52832) platform. Based on arduino-nRF5(https://github.com/sandeepmistry/arduino-nRF5).

The Radio is working in an NRF24 compatible mode. Other protocols can implemented. SoftDevice (BLE) is not supported and must be removed before flashing MySensors.

The included "NVM" driver allows an EEPROM like access to the last 16/32k of Flash pages. It can easily ported to ESP8266 or SAMD by providing an Flash interface.

Documentation about using this code: https://www.openhardware.io/view/376/MySensors-NRF5-Platform

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

@henrikekblad
Copy link
Member

Great job @d00616!

@d00616
Copy link
Contributor Author

d00616 commented Apr 23, 2017 via email

@tekka007 tekka007 added the nRF5 label Apr 23, 2017
@fallberg
Copy link
Contributor

Nice work! Once submitted I'll at some point update jenkins to do some sanity checking on this variant so we get static code analysis and doxygen validation on it fully covered. I might make various patches to the code at that point to get rid of the typical issues but they are usually confined to comment blocks.

@tekka007 tekka007 requested review from tekka007 and scalz April 29, 2017 10:43
@tekka007 tekka007 added this to the 2.2.0 milestone Apr 30, 2017
Copy link
Contributor

@tekka007 tekka007 left a comment

Choose a reason for hiding this comment

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

Well done @d00616 - worked out of the box :)
(Port tested using nRF51-DK dongle and nRF52-DK)

Please update hwDebugPrint() to include the most recent changes (timestamp), see here for reference https://github.com/mysensors/MySensors/blob/development/hal/architecture/MyHwAVR.cpp#L269-L295

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@d00616
Copy link
Contributor Author

d00616 commented Apr 30, 2017

@tekka007 hwDebugPrint() is changed. I have added the hwCPUVoltage() functionality.

If nothing more to change, I merge my last commit with 2ae764b

// already be handled!
MY_CRITICAL_SECTION {
// Fix cppcheck "(style) Variable '__savePriMask' is assigned a value that is never used."
(void)__savePriMask;
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 don't have any better idea about getting rid this message. This needs a fix in MyHwNRF5.h and MyHwSAMD.h

Copy link
Contributor

Choose a reason for hiding this comment

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

(void) casting in these situations is an acceptable solution

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) and/or coding style not good enough. I have mailed the details to the PR author.

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) and/or coding style not good enough. I have mailed the details to the PR author.

This is an EEPROM replacement based on MCU's internal Flash memory.
The NVRAM class is designed to be stateless. Each operation is
written to flash immediately.

The last 16k(nRF51)/32k(nRF52) of Flash memory are managed by the
VirtualPage class.  This class allows writing a single emulated 4k
page up 40,000 times.

The NVRAM class is a log based byte-wise storage allowing up to
40,000,000 writes depending on the number of used addresses.  Each
four allocated addresses from the beginning of the NVRAM reduce the
write log by one.

When first 256 bytes of the emulated storage are used, there is an
average writing capacity of 145,000 cycles per byte.

When the write log is full, the NVRAM is compressed into a new flash
page. After a new page is ready, the old page is released. This
process can take up to 4500 ms and cannot be interrupted.

The NVRAM functionality can easily be ported to ESP8266 or SAMD by
providing a Flash interface.
@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@d00616
Copy link
Contributor Author

d00616 commented May 6, 2017

Now I have merged the review changes into 9e9a03f, so there are two clear commits.

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

Copy link
Contributor

@tekka007 tekka007 left a comment

Choose a reason for hiding this comment

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

@d00616 I've tested your port in greater detail, please see the comments


// build metadata
tx_buffer.len = len;
tx_buffer.noack = noACK;
Copy link
Contributor

Choose a reason for hiding this comment

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

The RF24 driver sets the NOACK flag either for broadcasted messages or if the noACK parameter is set (used for passive node mode) - I suggest this change:

tx_buffer.noack = noACK || recipient==BROADCAST_ADDRESS;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tekka007 Changed. If you are fine with this change, I merge the Commit into the main commit.

tx_buffer.pid++;

// Calculate number of retries
tx_retries = ((recipient == BROADCAST_ADDRESS)?(NRF5_ESB_BC - 1):(NRF5_ESB_ARC - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

BC / noACK messages should be sent once.

A general thought: Many users have nRF24L01+ counterfeits with flipped noACK bits - this should be taken into consideration in a mixed (genuine/counterfeit) network, i.e. the SW ESB/RF24 noACK implementation should be adjustable, maybe as preprocessor switch?

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 added MY_NRF5_ESB_REVERSE_ACK

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

Copy link
Contributor

@tekka007 tekka007 left a comment

Choose a reason for hiding this comment

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

@d00616 Thanks for your fast replies and correction, much appreciated 👍 - I've just tested your last modifications - comments, see below

@@ -53,7 +53,7 @@
#define NRF5_ESB_ARC (15)

// How often broadcast messages are send
#define NRF5_ESB_BC (4)
#define NRF5_ESB_BC (1)
Copy link
Contributor

Choose a reason for hiding this comment

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

if set to (1) BC msgs won't be sent (due to this)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. It's fixed.

@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

This is the support of the Nordic Semiconductor NRF51 (nrf51822,
nrf51422) and NRF52 (nrf52832) platform. Based on arduino-nRF5
(https://github.com/sandeepmistry/arduino-nRF5).

The included radio driver is compatible with the NRF24 devices.
Using a SoftDevice (BLE)is not supported and must be removed before
flashing MySensors with a "mass_erase" operation.
@mysensors-jenkins
Copy link

Will evaluate git discipline...

@mysensors-jenkins
Copy link

Git commit message(s) look good!

@d00616
Copy link
Contributor Author

d00616 commented May 7, 2017

Commits merged.

@tekka007 Thank you for your review.

@mysensors-jenkins
Copy link

Core tests successful.
A member of the MySensors core team can ask me to verify this pull request more extensively by telling me "jenkins, build examples please".
But remember that I will only listen if you name me with a capital 'J'.

@tekka007 tekka007 merged commit 726a20d into mysensors:development May 14, 2017
@d00616 d00616 deleted the add_nrf5_platform branch August 29, 2017 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants