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

WIP: feat: `rubble-nrf51`/nRF51 support #59

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@chocol4te
Copy link
Collaborator

commented May 10, 2019

Closes #25.

Following must be complete before we can merge:

  • jamesmunns/bbqueue#27 needs to be merged, and the git dependency removed
  • New log release so we can remove that git dependency
  • Build this configuration in CI
  • Test connecting and enumerating services on real hardware
  • Add nrf52810 support to rubble-demo

@chocol4te chocol4te changed the title Feat: `rubble-nrf51`/nRF51 support WIP: feat: `rubble-nrf51`/nRF51 support May 10, 2019

@jonas-schievink
Copy link
Owner

left a comment

Looks pretty good, nice to support these chips again! Would be nice to share more code between the nrf51 and nrf52 implementations though :/

Show resolved Hide resolved rubble-nrf51/src/lib.rs Outdated
Show resolved Hide resolved rubble-nrf51/src/timer.rs
Show resolved Hide resolved rubble/src/lib.rs Outdated
Show resolved Hide resolved rubble/Cargo.toml Outdated

@chocol4te chocol4te force-pushed the chocol4te:master branch from 7546d02 to f311c1a Jun 11, 2019

Show resolved Hide resolved rubble-nrf51/src/timer.rs Outdated
Show resolved Hide resolved rubble-nrf51/src/timer.rs Outdated
@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 11, 2019

Another thing to do before merging: Test the demo on actual nRF51 hardware, making sure that connection and service enumeration works.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2019

Another thing to do before merging: Test the demo on actual nRF51 hardware, making sure that connection and service enumeration works.

Doing that now, CI should also be testing that it builds on thumbv6 targets

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

So I modified the demo slightly for the nRF51, rubble-demo-nrf51.

There appear to be 2 issues.

The first is that logging does not appear to be working. Serial works, as <<< INIT >>> appears, but this line is never called.

The second is that connecting hangs, but due to issue number 1, I don't really know how to go about diagnosing it.

IMG_0286

IMG_0287

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

<< INIT >> is printed via semihosting, while the log messages use the UART

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Sorry I meant --- INIT ---, which gets printed after setting up the serial interface. Angle brackets INIT is printed over semihosting.

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

Maybe there's a bug in jamesmunns/bbqueue#27 that causes all data put in a bbqueue to get lost? Just a guess though.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Removed all the cfg's for logging, and now I get:

--- INIT ---
INFO - Logger ready
6.025ms -

Progress?

Still appears in nRF connect, connecting still hangs.

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

I'm pretty sure all those Relaxed orderings aren't strong enough. heapless puts a compiler_fence with the correct ordering between all ops.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

But log::info!("Logger ready"); is working using the bbqueue? Or is it that one message could work, several would fail?

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

I'll make those changes in my fork and see what happens :)

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

Well, it would probably cause a data race and thus undefined behavior, so the program can do anything. How is the RAM usage? IIRC the log buffer is quite large. Do you have 32 or 16 KiB RAM on the chip? You could be running out of stack space.

EDIT: Okay, the incorrect atomics wouldn't immediately be UB on their own, but bbqueue also manages the queue memory unsafely, and I can see that blowing up when the atomics don't always work correctly.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

The nRF51822 I have has 32K RAM, 256K flash. Maybe I should try that stack size tool?

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

No, the nRF52810 we were using only has 24K of RAM, so it can't be that

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

https://www.adafruit.com/product/2267

As of July 29th, 2015 we're selling an updated version with a black PCB and the nRF51822 module with 32KB of SRAM.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Release build has interesting output:

Reading symbols from target/thumbv6m-none-eabi/release/rubble-demo-nrf51...
Target voltage: unknown
Available Targets:
No. Att Driver
 1      Nordic nRF51
0x0002d730 in ?? ()
Loading section .vector_table, size 0xa8 lma 0x0
Loading section .text, size 0xd97a lma 0xa8
Loading section .rodata, size 0x55c8 lma 0xda40
Loading section .data, size 0xc8 lma 0x13008
Start address 0xd9c8, load size 78002
Transfer rate: 26 KB/sec, 951 bytes/write.

<< INIT >>

panicked at 'called `Result::unwrap()` on an `Err` value: WouldBlock', src/libcore/result.rs:997:5

Program received signal SIGTRAP, Trace/breakpoint trap.
0x0000ac6c in __bkpt ()
(gdb)
@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

<< INIT >>

panicked at 'there is no such thing as an acquire/release load', src/libcore/sync/atomic.rs:2127:19

????

https://doc.rust-lang.org/core/sync/atomic/enum.Ordering.html#variant.AcqRel

Has the effects of both Acquire and Release together: For loads it uses Acquire ordering. For stores it uses the Release ordering.

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

This ordering is only applicable for operations that combine both loads and stores.

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

🤦‍♂

@chocol4te chocol4te force-pushed the chocol4te:master branch from e1a1acc to e6310f7 Jun 13, 2019

@chocol4te

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

--- INIT ---
INFO - Logger ready
6.037ms -

Putting compiler fences everywhere doesn't appeared to have worked :(

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

Hmm, then I'm not sure what could cause this. I do remember seeing this myself, however (on a nRF52810). Not sure why it happened or how it got fixed.

@jonas-schievink

This comment has been minimized.

Copy link
Owner

commented Jun 13, 2019

The advertisement is showing up continuously though? Then there's definitely something wrong with the log queue not being filled/drained properly (once it's full it should panic). Or is the UART going silent?

chocol4te and others added some commits Apr 30, 2019

Update rubble-nrf51/src/timer.rs
Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>
Update rubble-nrf51/src/timer.rs
Co-Authored-By: Jonas Schievink <jonasschievink@gmail.com>

@chocol4te chocol4te force-pushed the chocol4te:master branch from 464bf20 to 5fe38b0 Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.