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

Implement simple radio module to send/recv strings. #283

Merged
merged 7 commits into from Jul 10, 2016

Conversation

Projects
None yet
9 participants
@dpgeorge
Member

dpgeorge commented May 25, 2016

This is a first attempt at implementing a radio module. It's completely self contained (ie does not use the DAL) but the settings for the radio are compatible with the DAL's settings.

There are many things than can be improved/added/changed, but this is a good starting point to have a discussion about how the radio should be exposed in Python.

To use:

import radio
radio.enable(32, 4) # max payload is 32 bytes, queue length is 4
radio.send('hello!') # send a message to everyone listening
radio.recv() # receive any pending messages on the queue; returns None if none available

You'll need 2 or more microbits to try it out. It's a simple broadcast of messages and everyone uses the same channel and address (which can be configured but there's no such functionality yet).

One other thing: you'll need to edit the DAL to get this working. In yotta_modules/microbit-dal/source/ble-services/MicroBitRadio.cpp delete the entire RADIO_IRQHandler function between lines 29 and 57.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll May 25, 2016

Contributor

OK... I'm about to go to bed (up at 2:30am to drive to Heathrow). Testing the above on my 13 hour flight will make the hours (literally) fly by. Many many thanks @dpgeorge. :-)

Contributor

ntoll commented May 25, 2016

OK... I'm about to go to bed (up at 2:30am to drive to Heathrow). Testing the above on my 13 hour flight will make the hours (literally) fly by. Many many thanks @dpgeorge. :-)

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon May 25, 2016

Collaborator

Nice feature. Should be fun to play with.

A few comments.

  • I would prefer receive to recv.
  • I notice that send() takes any buffer object, but recv() returns bytes objects. That means that send('µ') will cause b'\xc2\xb5' to be received. Would it better if only we allowed strings?
  • I think the non-blocking nature of recv might make it a bit difficult to use. I would prefer it to be blocking, with non-blocking as an option: receive(block=True)
Collaborator

markshannon commented May 25, 2016

Nice feature. Should be fun to play with.

A few comments.

  • I would prefer receive to recv.
  • I notice that send() takes any buffer object, but recv() returns bytes objects. That means that send('µ') will cause b'\xc2\xb5' to be received. Would it better if only we allowed strings?
  • I think the non-blocking nature of recv might make it a bit difficult to use. I would prefer it to be blocking, with non-blocking as an option: receive(block=True)
@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll May 26, 2016

Contributor

(From Heathrow terminal 4)

@markshannon, +1 on receive although I hope all the 11yo remember it's "i before e except before c" etc.... ;-)

I'd prefer to be able to send and receive strings / bytes. In terms of receiving messages, to avoid the potential bump having to explain UTF-8 to 11yo (or anyone, for that matter) could the incoming message be an object with .text() and '.bytes()` methods? I'm not 100% certain this is the best solution so putting it out there for push-back / alternatives.

Finally, non-blocking, if implemented, should be consistent with the other modules so, wait=False as in the display and music modules.

I'll sketch up fake API docs on a branch so we can work out how best to express these things.

Contributor

ntoll commented May 26, 2016

(From Heathrow terminal 4)

@markshannon, +1 on receive although I hope all the 11yo remember it's "i before e except before c" etc.... ;-)

I'd prefer to be able to send and receive strings / bytes. In terms of receiving messages, to avoid the potential bump having to explain UTF-8 to 11yo (or anyone, for that matter) could the incoming message be an object with .text() and '.bytes()` methods? I'm not 100% certain this is the best solution so putting it out there for push-back / alternatives.

Finally, non-blocking, if implemented, should be consistent with the other modules so, wait=False as in the display and music modules.

I'll sketch up fake API docs on a branch so we can work out how best to express these things.

@c0d3st0rm

This comment has been minimized.

Show comment
Hide comment
@c0d3st0rm

c0d3st0rm May 26, 2016

Contributor

Radio? On a plane?

Now, why did that ring a bell ;)

On Thu, 26 May 2016, 05:39 Nicholas Tollervey, notifications@github.com
wrote:

(From Heathrow terminal 4)

@markshannon https://github.com/markshannon, +1 on receive although I
hope all the 11yo remember it's "i before e except before c" etc.... ;-)

I'd prefer to be able to send and receive strings / bytes. In terms of
receiving messages, to avoid the potential bump having to explain UTF-8 to
11yo (or anyone, for that matter) could the incoming message be an object
with .text() and '.bytes()` methods? I'm not 100% certain this is the
best solution so putting it out there for push-back / alternatives.

Finally, non-blocking, if implemented, should be consistent with the other
modules so, wait=False as in the display and music modules.

I'll sketch up fake API docs on a branch so we can work out how best to
express these things.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#283 (comment)

Contributor

c0d3st0rm commented May 26, 2016

Radio? On a plane?

Now, why did that ring a bell ;)

On Thu, 26 May 2016, 05:39 Nicholas Tollervey, notifications@github.com
wrote:

(From Heathrow terminal 4)

@markshannon https://github.com/markshannon, +1 on receive although I
hope all the 11yo remember it's "i before e except before c" etc.... ;-)

I'd prefer to be able to send and receive strings / bytes. In terms of
receiving messages, to avoid the potential bump having to explain UTF-8 to
11yo (or anyone, for that matter) could the incoming message be an object
with .text() and '.bytes()` methods? I'm not 100% certain this is the
best solution so putting it out there for push-back / alternatives.

Finally, non-blocking, if implemented, should be consistent with the other
modules so, wait=False as in the display and music modules.

I'll sketch up fake API docs on a branch so we can work out how best to
express these things.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#283 (comment)

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Jun 3, 2016

Collaborator

Some thoughts on the API:
We should drop enable() altogether. receive() should create any buffers it needs on demand, using the current configuration, and we should add a configure() method to allow those settings to be modified.

As to the default config, what would be good values?

Collaborator

markshannon commented Jun 3, 2016

Some thoughts on the API:
We should drop enable() altogether. receive() should create any buffers it needs on demand, using the current configuration, and we should add a configure() method to allow those settings to be modified.

As to the default config, what would be good values?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jun 3, 2016

Member

We should drop enable() altogether.

Enable is needed to turn on the receiver. Otherwise you don't get any packets. After enabled() is called the microbit is listening in the background and stores any incoming packets into the buffer (the one that you create with enable()). Then recv() just gets anything in the buffer.

Member

dpgeorge commented Jun 3, 2016

We should drop enable() altogether.

Enable is needed to turn on the receiver. Otherwise you don't get any packets. After enabled() is called the microbit is listening in the background and stores any incoming packets into the buffer (the one that you create with enable()). Then recv() just gets anything in the buffer.

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Jun 3, 2016

Collaborator

The receiver could be enabled by importing the module.
If not, then we should provide sensible defaults to the enable() function.

Collaborator

markshannon commented Jun 3, 2016

The receiver could be enabled by importing the module.
If not, then we should provide sensible defaults to the enable() function.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jun 3, 2016

Member

I think the cleanest thing to do is have on/off functionality for the receiver. The receiver takes power and RAM (for buffers) and maybe you only want it on at certain points in your code.

In principle you can send() without the receiver being enabled but it's not coded that way at the moment (actually, you can only send when the receiver is off, but the code at the moment assumes the receiver is on when you call send, and turns it back on once the send is finished).

Member

dpgeorge commented Jun 3, 2016

I think the cleanest thing to do is have on/off functionality for the receiver. The receiver takes power and RAM (for buffers) and maybe you only want it on at certain points in your code.

In principle you can send() without the receiver being enabled but it's not coded that way at the moment (actually, you can only send when the receiver is off, but the code at the moment assumes the receiver is on when you call send, and turns it back on once the send is finished).

@c-mart

This comment has been minimized.

Show comment
Hide comment
@c-mart

c-mart Jun 5, 2016

Contributor

Lots of fun with this at the PyCon sprint session yesterday. @octaflop and I were at the same table but working on different things, and he could raise exceptions on my micro:bit by transmitting packets that my program did not understand. I resolved this by prefixing all my transmissions with a specific header value, and discarding any received packets which didn't bear this value (example).

There are lots of ways to handle this kind of interference, but I think it's most interesting to keep the API pretty simple and let the MicroPython programmer figure it out. Students could implement simple forms of addressing, recipient groups, and other ways to limit the broadcast domain.

It'd be nice if the API exposed a facility to choose different, non-overlapping frequencies. That way, @octaflop and I could work on our programs without undesired interference.

Contributor

c-mart commented Jun 5, 2016

Lots of fun with this at the PyCon sprint session yesterday. @octaflop and I were at the same table but working on different things, and he could raise exceptions on my micro:bit by transmitting packets that my program did not understand. I resolved this by prefixing all my transmissions with a specific header value, and discarding any received packets which didn't bear this value (example).

There are lots of ways to handle this kind of interference, but I think it's most interesting to keep the API pretty simple and let the MicroPython programmer figure it out. Students could implement simple forms of addressing, recipient groups, and other ways to limit the broadcast domain.

It'd be nice if the API exposed a facility to choose different, non-overlapping frequencies. That way, @octaflop and I could work on our programs without undesired interference.

@octaflop

This comment has been minimized.

Show comment
Hide comment
@octaflop

octaflop Jun 5, 2016

Contributor

😀👍

Glad I was able to break your code.

Contributor

octaflop commented Jun 5, 2016

😀👍

Glad I was able to break your code.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jun 5, 2016

Member

It'd be nice if the API exposed a facility to choose different, non-overlapping frequencies. That way, @octaflop and I could work on our programs without undesired interference.

There is a radio address that is set when you call enable() that does filtering of incoming packets at the hardware level, keeping only those that match the address you set. At the moment all microbits use the same address and there's no way to change it. But certainly we should provide a method to get/set the address.

Member

dpgeorge commented Jun 5, 2016

It'd be nice if the API exposed a facility to choose different, non-overlapping frequencies. That way, @octaflop and I could work on our programs without undesired interference.

There is a radio address that is set when you call enable() that does filtering of incoming packets at the hardware level, keeping only those that match the address you set. At the moment all microbits use the same address and there's no way to change it. But certainly we should provide a method to get/set the address.

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Jun 5, 2016

Collaborator

Further thoughts on the API:
It would be nice to be able to configure the various radio parameters.
It would also nice to be able to not worry about the parameters and just call radio.enable().

I would suggest adding extra parameters to the enable function with defaults for everything.
For example:
radio.enable(length=32, queue=3, channel = 7, power=0, address=b'uBit\x00', data_rate=radio.MODE_1MHZ)

Collaborator

markshannon commented Jun 5, 2016

Further thoughts on the API:
It would be nice to be able to configure the various radio parameters.
It would also nice to be able to not worry about the parameters and just call radio.enable().

I would suggest adding extra parameters to the enable function with defaults for everything.
For example:
radio.enable(length=32, queue=3, channel = 7, power=0, address=b'uBit\x00', data_rate=radio.MODE_1MHZ)

@whaleygeek

This comment has been minimized.

Show comment
Hide comment
@whaleygeek

whaleygeek Jun 5, 2016

Collaborator

Just a tiny comment:

Is this enable(), or is this init() ??? (Just thinking of the other discussions about the SPI and I2C modules, which use init() to configure the peripheral. enable() to me sounds like 'turn it on' rather than 'set it up'??

Collaborator

whaleygeek commented Jun 5, 2016

Just a tiny comment:

Is this enable(), or is this init() ??? (Just thinking of the other discussions about the SPI and I2C modules, which use init() to configure the peripheral. enable() to me sounds like 'turn it on' rather than 'set it up'??

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jun 6, 2016

Contributor

On 05/06/16 23:28, David Whale wrote:

Just a tiny comment:

Is this enable(), or is this init() ??? (Just thinking of the other
discussions about the SPI and I2C modules, which use init() to configure
the peripheral. enable() to me sounds like 'turn it on' rather than 'set
it up'??


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#283 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACS4jzWLQXSxisdGwCfm3OkUrm5wjDmks5qI02XgaJpZM4ImnK1.

This is a "turn on" rather than "set up" afaict.

N.

Contributor

ntoll commented Jun 6, 2016

On 05/06/16 23:28, David Whale wrote:

Just a tiny comment:

Is this enable(), or is this init() ??? (Just thinking of the other
discussions about the SPI and I2C modules, which use init() to configure
the peripheral. enable() to me sounds like 'turn it on' rather than 'set
it up'??


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#283 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AACS4jzWLQXSxisdGwCfm3OkUrm5wjDmks5qI02XgaJpZM4ImnK1.

This is a "turn on" rather than "set up" afaict.

N.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jun 6, 2016

Contributor

Folks, I'll set up a "straw man" API description in the doc folder on a branch. I'll fill it in with a summary of the discussion above and we can work out the details on the PR.

Contributor

ntoll commented Jun 6, 2016

Folks, I'll set up a "straw man" API description in the doc folder on a branch. I'll fill it in with a summary of the discussion above and we can work out the details on the PR.

@markshannon

This comment has been minimized.

Show comment
Hide comment
@markshannon

markshannon Jun 21, 2016

Collaborator

I was thinking about the whole bytes/strings API thing.
Here is my suggestion:
You receive whatever was sent (like pickling), but limited to string and bytes.
I send a string, you recv a string. I send bytes, you receive bytes. Python is a dynamically typed language after all.
(We might want to support ints and None, but lets save that for another PR).

We can use the "logical address" to determine the type, with 1 representing the appropriate type to interoperate with the DAL. I presume that means bytes.

Collaborator

markshannon commented Jun 21, 2016

I was thinking about the whole bytes/strings API thing.
Here is my suggestion:
You receive whatever was sent (like pickling), but limited to string and bytes.
I send a string, you recv a string. I send bytes, you receive bytes. Python is a dynamically typed language after all.
(We might want to support ints and None, but lets save that for another PR).

We can use the "logical address" to determine the type, with 1 representing the appropriate type to interoperate with the DAL. I presume that means bytes.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 9, 2016

Member

I updated this PR to implement the new radio API as discussed in #305. But there were a few small changes to the API that I made to keep things simple. The full API that I implemented is:

# initialisation of module upon import, calls radio.reset()
__init__()

# reset configuration to default values
reset()

# configure the radio settings:
#  - length: max length of received packet
#  - queue: max number of outstanding received packets to hold on to
#  - power: power level between 0 and 7 inclusive
#  - channel: channel/freq between 0 and 100 inclusive
#  - data_rate: one of RATE_250KBIT, RATE_1MBIT, RATE_2MBIT (NOTE: names all upper case)
#  - address: 32-bit address to send/recv (NOTE: changed from a string to a number)
#  - group: 8-bit group to send/recv (NOTE: added to match DAL/PXT)
config(length, queue, power, channel, data_rate, address, group)

# turn on/off
on()
off()

# send/recv bytes
send_bytes(b)
receive_bytes()

# send/recv strings
send(s)
receive()

# constants
RATE_250KBIT
RATE_1MBIT
RATE_2MBIT

The way to set addresses is now changed from config(address='abcde') to config(address=0x12345678, group=42) to match DAL/PXT which has a setGroup() method to change just the group (0-255) without the base address (defaults to 'uBit'). So, conceptually, "address" is like a house/office address and "group" is like the person at that address that you want to send to. Arguably they could be named better, suggestions welcome!

I tested this with 2 microbits running uPy and it works. I also tested it with 1 microbit running uPy and the other running some code from PXT, and both send and receive work. NOTE: to get PXT to work you need to explicitly call radio.setGroup() in the PXT code, otherwise it uses a random group based on the hash of the script name.

Finally, note that you still need to edit the DAL code to comment out their radio IRQ function; see first post of this PR for details (we'll need to work out a way to handle this properly...).

Member

dpgeorge commented Jul 9, 2016

I updated this PR to implement the new radio API as discussed in #305. But there were a few small changes to the API that I made to keep things simple. The full API that I implemented is:

# initialisation of module upon import, calls radio.reset()
__init__()

# reset configuration to default values
reset()

# configure the radio settings:
#  - length: max length of received packet
#  - queue: max number of outstanding received packets to hold on to
#  - power: power level between 0 and 7 inclusive
#  - channel: channel/freq between 0 and 100 inclusive
#  - data_rate: one of RATE_250KBIT, RATE_1MBIT, RATE_2MBIT (NOTE: names all upper case)
#  - address: 32-bit address to send/recv (NOTE: changed from a string to a number)
#  - group: 8-bit group to send/recv (NOTE: added to match DAL/PXT)
config(length, queue, power, channel, data_rate, address, group)

# turn on/off
on()
off()

# send/recv bytes
send_bytes(b)
receive_bytes()

# send/recv strings
send(s)
receive()

# constants
RATE_250KBIT
RATE_1MBIT
RATE_2MBIT

The way to set addresses is now changed from config(address='abcde') to config(address=0x12345678, group=42) to match DAL/PXT which has a setGroup() method to change just the group (0-255) without the base address (defaults to 'uBit'). So, conceptually, "address" is like a house/office address and "group" is like the person at that address that you want to send to. Arguably they could be named better, suggestions welcome!

I tested this with 2 microbits running uPy and it works. I also tested it with 1 microbit running uPy and the other running some code from PXT, and both send and receive work. NOTE: to get PXT to work you need to explicitly call radio.setGroup() in the PXT code, otherwise it uses a random group based on the hash of the script name.

Finally, note that you still need to edit the DAL code to comment out their radio IRQ function; see first post of this PR for details (we'll need to work out a way to handle this properly...).

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 9, 2016

Contributor

@dpgeorge great stuff (and interesting to see how close - or not - I got). ;-)

Regarding the DAL, is this something we could ask them to disable with a flag at compile time..?

Contributor

ntoll commented Jul 9, 2016

@dpgeorge great stuff (and interesting to see how close - or not - I got). ;-)

Regarding the DAL, is this something we could ask them to disable with a flag at compile time..?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 9, 2016

Member

and interesting to see how close - or not - I got

We did the same thing wrt to putting the radio state in a structure. The config function was complicated though.

Regarding the DAL, is this something we could ask them to disable with a flag at compile time..?

Yes we will need to. Big problem though is that we are using an old version of the DAL (1.4.15), so if they push a change we'll need to update to the latest version that has that change. That will require extra work and testing. We can ask them to update the 1.4.x branch (currently at 1.4.17, ask for 1.4.18 with our patch) and then it'll be easier to upgrade. Otherwise we need to update to 2.0.0-rc4 which has benefits (component refactoring, reduced RAM usage) and drawbacks (lots more changes to test, and persistent state is stored right in the middle of our filesystem).

Member

dpgeorge commented Jul 9, 2016

and interesting to see how close - or not - I got

We did the same thing wrt to putting the radio state in a structure. The config function was complicated though.

Regarding the DAL, is this something we could ask them to disable with a flag at compile time..?

Yes we will need to. Big problem though is that we are using an old version of the DAL (1.4.15), so if they push a change we'll need to update to the latest version that has that change. That will require extra work and testing. We can ask them to update the 1.4.x branch (currently at 1.4.17, ask for 1.4.18 with our patch) and then it'll be easier to upgrade. Otherwise we need to update to 2.0.0-rc4 which has benefits (component refactoring, reduced RAM usage) and drawbacks (lots more changes to test, and persistent state is stored right in the middle of our filesystem).

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 9, 2016

Contributor

Hmm... OK... so the path of least resistance is a patch to 1.4.x of the DAL. If their code is modular though, surely we should be able to switch off their persistent state..? @finneyj @jamesadevine thoughts..?

Contributor

ntoll commented Jul 9, 2016

Hmm... OK... so the path of least resistance is a patch to 1.4.x of the DAL. If their code is modular though, surely we should be able to switch off their persistent state..? @finneyj @jamesadevine thoughts..?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 9, 2016

Member

so the path of least resistance is a patch to 1.4.x of the DAL

Yes I think we should do that to get this feature out the door ASAP.

If their code is modular though, surely we should be able to switch off their persistent state..?

Compass calibration relies on persistent state, and we want compass calibration.

Member

dpgeorge commented Jul 9, 2016

so the path of least resistance is a patch to 1.4.x of the DAL

Yes I think we should do that to get this feature out the door ASAP.

If their code is modular though, surely we should be able to switch off their persistent state..?

Compass calibration relies on persistent state, and we want compass calibration.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 9, 2016

Contributor

WRT compass calibration, one of the folks at PyCon was creating a non-DAL compass calibration. Hmm... I'll look back through my emails / twitter steam and see if I can ping him.

Contributor

ntoll commented Jul 9, 2016

WRT compass calibration, one of the folks at PyCon was creating a non-DAL compass calibration. Hmm... I'll look back through my emails / twitter steam and see if I can ping him.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 9, 2016

Contributor

OK... so can we create a patch for a release 1.4.18..? Is this as simple as adding a conditional around the problematic code. My naive / inexperienced C programming suggestion is for a new flag in inc/MicroBitConfig.h (perhaps MICROBIT_RADIO) which we override in our own inc/microbit/MicroBitCustomConfig.h and then with #ifdef MICROBIT_RADIO / #endif surrounding the problematic code in source/ble-services/MicroBitRadio.cpp. Is it really that simple..?

Contributor

ntoll commented Jul 9, 2016

OK... so can we create a patch for a release 1.4.18..? Is this as simple as adding a conditional around the problematic code. My naive / inexperienced C programming suggestion is for a new flag in inc/MicroBitConfig.h (perhaps MICROBIT_RADIO) which we override in our own inc/microbit/MicroBitCustomConfig.h and then with #ifdef MICROBIT_RADIO / #endif surrounding the problematic code in source/ble-services/MicroBitRadio.cpp. Is it really that simple..?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 9, 2016

Member

Is it really that simple..?

Yes! That would be the minimal thing to do. But conceptually you'd probably want to #ifdef out the entire radio module (including all the code in MicroBitRadioDatagram.cpp and MicroBitRadioEvent.cpp) since the radio won't work at all without the IRQ function. And removing the radio module will save a little bit of RAM and code size.

Member

dpgeorge commented Jul 9, 2016

Is it really that simple..?

Yes! That would be the minimal thing to do. But conceptually you'd probably want to #ifdef out the entire radio module (including all the code in MicroBitRadioDatagram.cpp and MicroBitRadioEvent.cpp) since the radio won't work at all without the IRQ function. And removing the radio module will save a little bit of RAM and code size.

@finneyj

This comment has been minimized.

Show comment
Hide comment
@finneyj

finneyj Jul 9, 2016

Collaborator

Guys, we could patch 1.4.x if really needed but we've really left that behind since microbit.co.uk was locked down. In 2.x you could simply just not include the radio component in your build, which would be a lot cleaner.

Seems a shame to duplicate stuff though... if you've got a better, compatible implementation, we could consider upstreaming/merging it so everyone's running the same core? We'd welcome improvements...

Collaborator

finneyj commented Jul 9, 2016

Guys, we could patch 1.4.x if really needed but we've really left that behind since microbit.co.uk was locked down. In 2.x you could simply just not include the radio component in your build, which would be a lot cleaner.

Seems a shame to duplicate stuff though... if you've got a better, compatible implementation, we could consider upstreaming/merging it so everyone's running the same core? We'd welcome improvements...

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 9, 2016

Member

In 2.x you could simply just not include the radio component in your build, which would be a lot cleaner.

I don't think that would work because the function in question (RADIO_IRQHandler) has a special name that we also use and there is a clash.

if you've got a better, compatible implementation, we could consider upstreaming/merging it so everyone's running the same core?

It's not compatible, that's the problem. The main thing is that we use our own heap to allocate the buffers for packet reception, and we don't allocate during the IRQ.

Member

dpgeorge commented Jul 9, 2016

In 2.x you could simply just not include the radio component in your build, which would be a lot cleaner.

I don't think that would work because the function in question (RADIO_IRQHandler) has a special name that we also use and there is a clash.

if you've got a better, compatible implementation, we could consider upstreaming/merging it so everyone's running the same core?

It's not compatible, that's the problem. The main thing is that we use our own heap to allocate the buffers for packet reception, and we don't allocate during the IRQ.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2016

Member

Thanks heaps @finneyj! I'm using v1.4.19 now and it works perfectly.

I have rebased this PR against the latest master, added a commit to update the DAL to v1.4.19, fixed the qstr issues and tested it. The API is pretty much what we've decided upon and I think anything left to do will now be classified as a "bug", to be fixed later on. So feel free to merge after testing that it does actually compile and work :)

Member

dpgeorge commented Jul 10, 2016

Thanks heaps @finneyj! I'm using v1.4.19 now and it works perfectly.

I have rebased this PR against the latest master, added a commit to update the DAL to v1.4.19, fixed the qstr issues and tested it. The API is pretty much what we've decided upon and I think anything left to do will now be classified as a "bug", to be fixed later on. So feel free to merge after testing that it does actually compile and work :)

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

@dpgeorge results of this morning's radio related hacking to be on YouTube in 2mins... seriously good fun. ;-)

Will merge once YouTube faff is done.

Contributor

ntoll commented Jul 10, 2016

@dpgeorge results of this morning's radio related hacking to be on YouTube in 2mins... seriously good fun. ;-)

Will merge once YouTube faff is done.

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2016

Member

I'd really like to see if there's any hope of at least some driver sharing between languages before we all diverge too far though.

The main things we need are control over the memory allocation for incoming packets (ie all allocation is done at set up), and the ability to send (and receive) raw bytes, not a FrameBuffer, eg send(const char *buf, size_t len).

Member

dpgeorge commented Jul 10, 2016

I'd really like to see if there's any hope of at least some driver sharing between languages before we all diverge too far though.

The main things we need are control over the memory allocation for incoming packets (ie all allocation is done at set up), and the ability to send (and receive) raw bytes, not a FrameBuffer, eg send(const char *buf, size_t len).

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

@dpgeorge done with the help of Penelope: https://www.youtube.com/watch?v=kPW_lf0P7XU ;-)

Contributor

ntoll commented Jul 10, 2016

@dpgeorge done with the help of Penelope: https://www.youtube.com/watch?v=kPW_lf0P7XU ;-)

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2016

Member

Cooool!

Member

dpgeorge commented Jul 10, 2016

Cooool!

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

It won't build. :-(

Looking into it now.

Contributor

ntoll commented Jul 10, 2016

It won't build. :-(

Looking into it now.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

I have correct DAL, but I get:

collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
error: command ['ninja'] failed
Makefile:12: recipe for target 'yotta' failed
make: *** [yotta] Error 1

I don't see any obvious errors in the output spewed out by the compilation process. Will look into this further. Builds fine on master. :-/

Contributor

ntoll commented Jul 10, 2016

I have correct DAL, but I get:

collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
error: command ['ninja'] failed
Makefile:12: recipe for target 'yotta' failed
make: *** [yotta] Error 1

I don't see any obvious errors in the output spewed out by the compilation process. Will look into this further. Builds fine on master. :-/

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

Does this mean anything to you..?

/usr/lib/gcc/arm-none-eabi/4.9.3/../../../arm-none-eabi/bin/ld: region RAM overflowed with stack
Contributor

ntoll commented Jul 10, 2016

Does this mean anything to you..?

/usr/lib/gcc/arm-none-eabi/4.9.3/../../../arm-none-eabi/bin/ld: region RAM overflowed with stack
@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2016

Member

Delete your build/ directory, run yt target again, yt up, then make.

What version of arm-none-eabi-gcc do you use?

Member

dpgeorge commented Jul 10, 2016

Delete your build/ directory, run yt target again, yt up, then make.

What version of arm-none-eabi-gcc do you use?

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

OK, still fails. I have:

  • deleted build/ and yotta_* directories.
  • run yt target bbc-microbit-classic-gcc-nosd
  • run yt up
  • run make

GCC version is: 4.9.3 on latest stable Ubuntu (it all works on master).

Final message is:

/usr/lib/gcc/arm-none-eabi/4.9.3/../../../arm-none-eabi/bin/ld: region RAM overflowed with stack
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
error: command ['ninja'] failed
Makefile:12: recipe for target 'yotta' failed
make: *** [yotta] Error 1
Contributor

ntoll commented Jul 10, 2016

OK, still fails. I have:

  • deleted build/ and yotta_* directories.
  • run yt target bbc-microbit-classic-gcc-nosd
  • run yt up
  • run make

GCC version is: 4.9.3 on latest stable Ubuntu (it all works on master).

Final message is:

/usr/lib/gcc/arm-none-eabi/4.9.3/../../../arm-none-eabi/bin/ld: region RAM overflowed with stack
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
error: command ['ninja'] failed
Makefile:12: recipe for target 'yotta' failed
make: *** [yotta] Error 1
@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 10, 2016

Member

Ok, the problem is that the newer version of the DAL uses about 24 bytes more RAM than the previous one we were using and that just overflows the total available RAM :( On my PC it build ok because I use gcc v6.1.1 which does more optimisations to reduce RAM usage (I assume).

In order to move things along I will push a change to this branch to reduce the uPy heap by 48 bytes, so there is room to grow a bit more in the future.

Member

dpgeorge commented Jul 10, 2016

Ok, the problem is that the newer version of the DAL uses about 24 bytes more RAM than the previous one we were using and that just overflows the total available RAM :( On my PC it build ok because I use gcc v6.1.1 which does more optimisations to reduce RAM usage (I assume).

In order to move things along I will push a change to this branch to reduce the uPy heap by 48 bytes, so there is room to grow a bit more in the future.

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

OK... that worked!

Contributor

ntoll commented Jul 10, 2016

OK... that worked!

@ntoll ntoll merged commit 45a4398 into bbcmicrobit:master Jul 10, 2016

@ntoll

This comment has been minimized.

Show comment
Hide comment
@ntoll

ntoll Jul 10, 2016

Contributor

Give me about an hour and I'll have a radio tutorial in #305 for us to merge.

Contributor

ntoll commented Jul 10, 2016

Give me about an hour and I'll have a radio tutorial in #305 for us to merge.

@finneyj

This comment has been minimized.

Show comment
Hide comment
@finneyj

finneyj Jul 19, 2016

Collaborator

@dpgeorge - thanks for clarifying, Been in the US for a bit, so sorry for slow reply. (Got a go in a Tesla though... so not that sorry! ;-)

I put these in for you guys when I wrote the radio interface:

https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitRadioDatagram.cpp#L66

https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitRadioDatagram.cpp#L119

Thought that would be good enough to do that job without the need to duplicate effort. Admittedly does have an additional copy op, but thought that would be OK for now... Does this sort of approach not work for you guys? if not, would be good to perhaps set up a few guidelines on what's good/not for python re-use, so we can try to align better in the future?

Collaborator

finneyj commented Jul 19, 2016

@dpgeorge - thanks for clarifying, Been in the US for a bit, so sorry for slow reply. (Got a go in a Tesla though... so not that sorry! ;-)

I put these in for you guys when I wrote the radio interface:

https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitRadioDatagram.cpp#L66

https://github.com/lancaster-university/microbit-dal/blob/master/source/drivers/MicroBitRadioDatagram.cpp#L119

Thought that would be good enough to do that job without the need to duplicate effort. Admittedly does have an additional copy op, but thought that would be OK for now... Does this sort of approach not work for you guys? if not, would be good to perhaps set up a few guidelines on what's good/not for python re-use, so we can try to align better in the future?

@dpgeorge dpgeorge deleted the dpgeorge:radio branch Jul 19, 2016

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 19, 2016

Member

Thought that would be good enough to do that job without the need to duplicate effort. Admittedly does have an additional copy op, but thought that would be OK for now... Does this sort of approach not work for you guys?

There are two big problems with that datagram implementation:

  1. It allocates/frees heap memory for each received packet. This is bad for determinism, fragmentation and efficiency. Also it uses the mbed/DAL heap and we want to use our own (so the size of the receive buffer is configurable by the user).

  2. It adds version and type bytes to the packets. This means you can't interoperate with other nRF devices that aren't based on the DAL. It's best just to let the user send and receive arbitrary bytes.

Member

dpgeorge commented Jul 19, 2016

Thought that would be good enough to do that job without the need to duplicate effort. Admittedly does have an additional copy op, but thought that would be OK for now... Does this sort of approach not work for you guys?

There are two big problems with that datagram implementation:

  1. It allocates/frees heap memory for each received packet. This is bad for determinism, fragmentation and efficiency. Also it uses the mbed/DAL heap and we want to use our own (so the size of the receive buffer is configurable by the user).

  2. It adds version and type bytes to the packets. This means you can't interoperate with other nRF devices that aren't based on the DAL. It's best just to let the user send and receive arbitrary bytes.

@finneyj

This comment has been minimized.

Show comment
Hide comment
@finneyj

finneyj Jul 19, 2016

Collaborator

Thanks @dpgeorge

I see your point re 1). Heap allocation is not very churny with the cache we have, but this would introduce some non-determinism in the interrupt time and heap size. I guess it's the latter you're worried about?

The rationale of the type and version was to try to preserve interop for microbit. so others could create different protocols within a little bit of structure... Those wanting other protocols could implement them side by side. Other nrf protocols (ANT/Gazelle etc) need very different low level different configs (CRC generation, freq hopping etc). If you're not adding these fields , won't you have interop issues with micro:bits using other languages?

Collaborator

finneyj commented Jul 19, 2016

Thanks @dpgeorge

I see your point re 1). Heap allocation is not very churny with the cache we have, but this would introduce some non-determinism in the interrupt time and heap size. I guess it's the latter you're worried about?

The rationale of the type and version was to try to preserve interop for microbit. so others could create different protocols within a little bit of structure... Those wanting other protocols could implement them side by side. Other nrf protocols (ANT/Gazelle etc) need very different low level different configs (CRC generation, freq hopping etc). If you're not adding these fields , won't you have interop issues with micro:bits using other languages?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 19, 2016

Member

If you're not adding these fields , won't you have interop issues with micro:bits using other languages?

We do use these fields for sending/receiving unicode strings, just so that we can be compatible with the other languages (eg PXT). But we also expose a lower-level API to send/receive a simple sequence of bytes without any header (the user can easily add a header to communicate with other DAL-based implementations). They can also change frequency, address, etc to have a lot of control over the radio.

Member

dpgeorge commented Jul 19, 2016

If you're not adding these fields , won't you have interop issues with micro:bits using other languages?

We do use these fields for sending/receiving unicode strings, just so that we can be compatible with the other languages (eg PXT). But we also expose a lower-level API to send/receive a simple sequence of bytes without any header (the user can easily add a header to communicate with other DAL-based implementations). They can also change frequency, address, etc to have a lot of control over the radio.

@finneyj

This comment has been minimized.

Show comment
Hide comment
@finneyj

finneyj Jul 20, 2016

Collaborator

cool. Perhaps we could look to upstream/merge the lower-level API? This would be good for maintenance and efficiency don't you think?

Collaborator

finneyj commented Jul 20, 2016

cool. Perhaps we could look to upstream/merge the lower-level API? This would be good for maintenance and efficiency don't you think?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 21, 2016

Member

Perhaps we could look to upstream/merge the lower-level API? This would be good for maintenance and efficiency don't you think?

Yes it would. But I think there are other, more important things to tackle. There's currently a lot of "wasted" memory that we would like to reclaim for the MicroPython heap. It would be great if we could completely eliminate the mbed/DAL heap altogether (saving 2k!). This would mean that the DAL components should idealy not use the heap, or should provide an init function through which you pass a pointer to the memory that they need.

For example, MicroBitPin can be rewritten to not use any RAM at all. Most state can be made ROMable (eg name, id, capabilities) and the rest (eg whether it's input/output/etc) can be inferred from the nRF GPIO registers. There should be no need to make dynamic Pin objects that take up precious heap memory. Would you be open to such changes?

Member

dpgeorge commented Jul 21, 2016

Perhaps we could look to upstream/merge the lower-level API? This would be good for maintenance and efficiency don't you think?

Yes it would. But I think there are other, more important things to tackle. There's currently a lot of "wasted" memory that we would like to reclaim for the MicroPython heap. It would be great if we could completely eliminate the mbed/DAL heap altogether (saving 2k!). This would mean that the DAL components should idealy not use the heap, or should provide an init function through which you pass a pointer to the memory that they need.

For example, MicroBitPin can be rewritten to not use any RAM at all. Most state can be made ROMable (eg name, id, capabilities) and the rest (eg whether it's input/output/etc) can be inferred from the nRF GPIO registers. There should be no need to make dynamic Pin objects that take up precious heap memory. Would you be open to such changes?

@finneyj

This comment has been minimized.

Show comment
Hide comment
@finneyj

finneyj Jul 21, 2016

Collaborator

@dpgeorge

Absolutely - I'd be very open to such optimizations, and would really welcome collaborating to make our support for micropython better. We need to make sure we don't break anything for mbed and PXT users on the way of course... but I'm very keen for micropython support to improve. Being quite different to the "online" languages that are cross compiling to microbit-dal, your needs really aren't well serviced at the moment.

Also agree that there are bigger wins to be had in places other than the radio driver. Just feeling out your interest here - really pleased you'd like to work to improve this together.

Eliminating all heap use would be a very ambitious goal, but for the subset of microbit-dal that would benefit to micropython (essentially driver API calls without refcounted types) it may be possible... and if its not possible to eliminate then to make the use more bounded (your idea above is a good example of this).

I'm afraid I'm just about to disappear on holiday for a few weeks though, so don't have time right now. I would very much like to keep discussing ideas when I get back though - if we can briefly talk through the principles and sketch out any necessary changes to APIs and/or assumptions before any PRs, it should be very straightforward for us to merge in changes. Sound good?

Collaborator

finneyj commented Jul 21, 2016

@dpgeorge

Absolutely - I'd be very open to such optimizations, and would really welcome collaborating to make our support for micropython better. We need to make sure we don't break anything for mbed and PXT users on the way of course... but I'm very keen for micropython support to improve. Being quite different to the "online" languages that are cross compiling to microbit-dal, your needs really aren't well serviced at the moment.

Also agree that there are bigger wins to be had in places other than the radio driver. Just feeling out your interest here - really pleased you'd like to work to improve this together.

Eliminating all heap use would be a very ambitious goal, but for the subset of microbit-dal that would benefit to micropython (essentially driver API calls without refcounted types) it may be possible... and if its not possible to eliminate then to make the use more bounded (your idea above is a good example of this).

I'm afraid I'm just about to disappear on holiday for a few weeks though, so don't have time right now. I would very much like to keep discussing ideas when I get back though - if we can briefly talk through the principles and sketch out any necessary changes to APIs and/or assumptions before any PRs, it should be very straightforward for us to merge in changes. Sound good?

@dpgeorge

This comment has been minimized.

Show comment
Hide comment
@dpgeorge

dpgeorge Jul 22, 2016

Member

@finneyj That does sound good! I also have very little time at the moment so we can pick this up in a few weeks.

Member

dpgeorge commented Jul 22, 2016

@finneyj That does sound good! I also have very little time at the moment so we can pick this up in a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment