-
-
Notifications
You must be signed in to change notification settings - Fork 284
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
In support of issue #98 (SoftwareSerial not working) #205
base: master
Are you sure you want to change the base?
In support of issue #98 (SoftwareSerial not working) #205
Conversation
…Register, portModeRegister # WInterrupts * change attachInterrupt define to return the interrupt mask * callbacksInt, channelMap now uses an initialiser list instead of memset * GPIOTE_IRQn priority dropped from 1 to 3 (same as Uart priority) * LOW and HIGH modes added to attachInterrupt for compatibility, which are the same as FALLING and RISING respectively as GPIOTE does not support HIGH and LOW modes * To free up clock cycles between GPIOTE interrupts GPIOTE_IRQHandler now searches for the first GPIOTE event and breaks to execute, rather than staying in the interrupt and executing each GPIOTE callback that has triggered an interrupt.
…ease 1.0.1 * merged SoftwareSerial.cpp into SoftwareSerial.h to allow ```_SS_MAX_RX_BUFF``` to be user-defined * added ```_SS_TX_ONLY``` user define to allow this to be used as a transmit only library (no interrupts used) * removed workarounds for legacy versions of Arduino that arent supported by this core
I noticed a comment from @sandeepmistry that you only wanted to include the core libraries so I added my SoftwareSerial variant. |
Which one is better? |
Im biased so ill say that my variant of the arduino-org one (which i added to this PR) is the best as it allows you to configure the buffer size without modifying the library, and you can configure it to be transmit only (saves a pin, a pin interrupt a little over 1kB of flash). To achieve this it is a header only library, so compile time is slightly longer. The arduino-org one was first, the adafruit one looks to be a direct copy. All three work, but not as well as the tuned AVR original source as the tuning on the bit timing is courser. |
Add support for SoftwareSerial library, from sandeepmistry#205
@@ -92,12 +92,12 @@ void loop( void ) ; | |||
|
|||
#define bit(b) (1UL << (b)) | |||
|
|||
#define digitalPinToPort(P) ( &(NRF_GPIO[P]) ) | |||
#define digitalPinToPort(P) ( NRF_GPIO ) // NRF_P0 = P0.00 - P0.31, NRF_P1 = P1.00 - P1.15 (e.g. nRF52840) |
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.
Could you clarify this? Sounds like to support the nRF52840 we need to evaluate P
to determine if this should be NRF_P0
or NRF_P1
(as the NRF_GPIO
macro is likely to return NRF_P0
).
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.
@carlosperate - you are correct. For the NRF52840 this would need to be rewritten.
One issue is that the pins on port 0 are 0-31, but i didnt believe the pins on port 1 were consistently reprrsented as 32 onwards
libraries/SoftwareSerial/README.md
Outdated
|
||
### License | ||
|
||
I do not claim copyright on the code, license taken from SoftwareSerial.h header. |
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 would be a bit confusing to have "I do not claim copyright..." here, as the reader cannot get any context from the source code alone.
IANAL and I honestly don't know enough about this stuff, but from what (I think) I understand, renouncing copyright can end up being problematic, as its possible that the new code doesn't fall under any software license, and so it's unclear if it can be freely modified and redistributed.
Once again, IANAL, but it should be fine to declare the original copyright for the original library, as you have done, and then say that the modifications listed are under the same license. Copyright for the changes should automatically fall under your name, but you don't have to add it if you don't want to.
paragraph=The SoftwareSerial library has been developed to allow serial communication on any digital pin of the board, using software to replicate the functionality of the hardware UART. It is possible to have multiple software serial ports with speeds up to 115200 bps. | ||
category=Communication | ||
url=http://arduino.cc/en/Reference/SoftwareSerial | ||
architectures=nrf52,nRF5 |
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 it advantageous/disadvantageous to have this file here? Honest question, don't know how this will play with the Arduino library manager.
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 library.properties file is for the Arduino library manager incase i decide to register this library with them. It also stops the library from being available to incompatible cores i.e. AVR.
name=SoftwareSerial | ||
version=1.1.0 | ||
author=Arduino | ||
maintainer=Mark Cooke (https://github/micooke) |
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.
Does this entry still apply?
@@ -0,0 +1,9 @@ | |||
name=SoftwareSerial | |||
version=1.1.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.
If this is a modification, should the version be changed?
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 is based on 1.0.1 of the Arduino library
#define _SS_TX_ONLY 1 | ||
Will stop the library allocating a GPIO interrupt and save 1364 bytes of memory | ||
*/ | ||
#define _SS_TX_ONLY 1 |
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 would be nice to also demonstrate the usage of _SS_MAX_RX_BUFF
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.
Will do
if (inv) b = ~b; | ||
|
||
// turn off interrupts for a clean txmit | ||
#ifndef _SS_TX_ONLY |
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.
_SS_TX_ONLY
is always defined, either by the user or as 0
at the top of the file.
There is a few of these around, search for ifndef
or ifdef
.
Has this been tested with nRF51s as well? |
@carlosperate - thankyou for your review! Sorry for not addressing each point in situ, my access is mobile only while on holidays. First up - thanks for spotting the Ive tested this against the nrf52832 and nrf51822, but it won't work for all pins of the nrf52840. The comment in Arduino.h is a hinter comment that more work is required for the nrf52840. Copyright is different to licensing, I have not/cannot changed the original license. I dont believe it is possible to sublicense within a source file, but i may be correct. As per your suggestion I'm happy to just add my name to the list for clarity, i just wanted to imply that the prior work was far more significant than my own hacks 😊 The library.properties file is for the Arduino library manager incase i decide to register this library with them. It also stops the library from being available to incompatible cores i.e. AVR. |
I can confirm that the code in this PR works with the micro:bit (nRF51822) using the included SoftwareSerial library. I was able to drive a Serial MP3 Player board off the micro:bit via a SoftwareSerial interface. Assuming that the above issues are resolved, it would be great if this PR (including SoftwareSerial) could be merged into master, as I've been working on a forked branch that requires both SoftwareSerial as well as other changes. Thanks in advance! |
@carlosperate @micooke Could you give me a status update on this PR? I am thinking we may be ready for another release in the near future. |
Im away for another week, ill be able to fix these minor changes by Friday |
Updates pushed earlier than expected.. |
* Added separate LICENSE file * Updated Copyright information * Bugfix: replaced #ifdef / #ifndef _SS_TX_ONLY with #if _SS_TX_ONLY == 1/0 * Updated TransmitOnly.ino to demonstrate the use of _SS_MAX_RX_BUFF
@dlabun, @sandeepmistry - Travis build failed, not sure why. It failed when it tries to get any packages; ubuntu packages, this repo from github.com. Very strange. I made a small update to trigger the build again - hopefully it is resolved by now!? |
I think it was just a temporary outage between Travis and Github, it's all good right now |
@micooke Travis is passing with this PR, any issues with merging it? |
Go for it 👍 |
Bump (I'm trying to clean up my PRs 😊) |
@dlabun I'm unsure what to do about this one. Concerns:
|
This was to put the priority the same as Uart, and lower than Wire interrupts. The concern is that GPIOTE can be triggered with a very high duty cycle, which could interfere with higher-priority tasking like I2C comms. On a related note, RTC interrupt priority is 15 in this core and 0 in the SAMD core. Just something to note as we are talking about interrupt priorities.
SoftwareSerial, but maybe we can diverge from this?
The changes i made to support SoftwareSerial are in line with modifications made by Arduino-org and Adafruit (who started with you core by the looks of it). You certainly dont need to use my variant of SoftwareSerial, the core changes make it compatible with the Adafruit and Arduino-org variants. |
This PR includes a SoftwareSerial library, and is compatible with the arduino-org and adafruit nrf52 SoftwareSerial libraries.
Arduino.h
WInterrupts