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

Radio is missing RSSI data #110

Open
pelikhan opened this issue Feb 3, 2020 · 9 comments
Open

Radio is missing RSSI data #110

pelikhan opened this issue Feb 3, 2020 · 9 comments
Assignees

Comments

@pelikhan
Copy link
Contributor

pelikhan commented Feb 3, 2020

How do I get the RSSI data from a radio packet?

@finneyj
Copy link
Contributor

finneyj commented Feb 4, 2020

Hmm... looks like some type erasure in codal-nrf52 c.f. the equivalent APIs we wrote for micro:bit.

https://github.com/lancaster-university/codal-nrf52/blob/master/source/NRF52RadioDatagram.cpp#L95

vs

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

I think we should either add generic meta data to a ManagedBuffer (might be useful for lots of things) or rebuild this equivalent APIs...

I think I quite like the first idea.

@pelikhan
Copy link
Contributor Author

pelikhan commented Feb 4, 2020

For backward compat, you should bring back PacketBuffer. I don’t think we want to bloat ManagedBuffer with metadata - it is used in many many places.

@pelikhan
Copy link
Contributor Author

pelikhan commented Feb 4, 2020

Or clone framebuffer, remove pointers and return it to me

@jamesadevine
Copy link
Contributor

I agree that we do not want to bloat ManagedBuffer.

I originally removed it because PacketBuffer seemed specialised to the micro:bit, only differing by its storing of the RSSI.

To me it makes sense to subclass ManagedBuffer as PacketBuffer. Existing APIs need not change, and code will not be duplicated.

@finneyj
Copy link
Contributor

finneyj commented Feb 4, 2020

I'm not so sure. I was just thinking of adding a uint16_t that could contain either dynamic type data for what's in that buffer (e.g. audio/network buffer). Could also cover audio formats if needed etc. etc.

Looking at the code, we're already 16 bits short of being word aligned anyway...
https://github.com/lancaster-university/codal-core/blob/master/inc/types/ManagedBuffer.h#L35

Maintaining PacketBuffer (even through a subclass) would add FLASH overhead.

@jamesadevine
Copy link
Contributor

Ha, nice. A subtype would make sense given our reuse of packet buffer for many different purposes.

I'd like to see the interface for using a subtyped buffer, application code would have to know the layout of the internal buffer structure. This is probably fine, but does introduce more error prone code.

@finneyj
Copy link
Contributor

finneyj commented Feb 4, 2020

I think i'd leave that to application code (or maybe some common struct definitions that you could cast etc etc). Just add the (optional) type info, and we could encode the RSSi in there.

If you want to go further we could have explicit trailer on the buffer that contained more metadata, but that's probably more complexity than we need right now.

@pelikhan
Copy link
Contributor Author

pelikhan commented Feb 4, 2020

Currently we append the RSSI to the packet data and send it to the typescript side — so you could just do that.

@finneyj
Copy link
Contributor

finneyj commented Feb 4, 2020

Yep. Score one for the meta data trailer idea. I'll do work something up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants