Skip to content
This repository has been archived by the owner on Jan 7, 2019. It is now read-only.

[feature] DW1000 Driver, 802.15.4 Frame & Ranging #361

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

Maju-Ketchup
Copy link
Contributor

Added driver for DW1000 with generic and STM32F4_disco examples
Added MAC Frame for IEEE 802.15.4 with unitttests
Added Ranging algorithms for UWB modules with unittests

It is one year since I started this project of including a DW1000 Ultra-Wide-Band Radio and I finally want you to merge it into the dev branch, so everyone can profit from it.
Everything is on discussion names, paths etc...

The code is tested and used at my workplace and the bugs are mostly fixed.
I see forward to an useful discussion and I know this merge request will take some time.
Please be aware that I do the public changes in my free-time of which I do not have much.

{
setdelayedtrxtime(time);
int result;
result = rxenable(1);
Copy link
Member

Choose a reason for hiding this comment

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

CircleCI fails with

src/xpcc/driver/radio/dw1000/dw1000_impl.hpp:853:22: error: no matching function for call to 'xpcc::Dw1000<Spi, Cs, Reset, Irq>::rxenable(int)'
src/xpcc/driver/radio/dw1000/dw1000.hpp:434:2: note: candidate: static int xpcc::Dw1000<Spi, Cs, Reset, Irq>::rxenable(xpcc::dw1000::RX_Mode)

This line should probably read as follows

result = rxenable(RX_Mode::START_RX_DELAYED);

(RX_Mode::START_RX_DELAYED == 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I changed using INTs to a custom enum some time ago. Seems to be a leftover.

@Maju-Ketchup Maju-Ketchup force-pushed the feature/DecaWaveDWM1000Driver branch 4 times, most recently from 2fb42f4 to 097cc98 Compare May 30, 2018 11:12
@rleh
Copy link
Member

rleh commented May 30, 2018

The atmega-unittests test in CircleCI fails because there is not ostream-operator for long long types for AVRs.

#if not defined(XPCC__CPU_AVR)
void
writeUnsignedLongLong(unsigned long long unsignedValue, uint_fast8_t base, size_t width, char fill, bool isNegative);
#endif

What is the reason for that? @salkinium

@salkinium
Copy link
Member

What is the reason for that?

Perfomance concerns on AVRs. The issue was that this will make ALL printf calls use 64-bit integer math, regardless of whether you need it or not.
#315

We can selectably enable this with a macro for AVR targets that want to use this. By default it would be limited to 32-bit integers, but if you really need this, you can add a such a define flag to the build and enable this. In modm this would then be enabled with a module option.

@Maju-Ketchup
Copy link
Contributor Author

I will disable unittests with 64bit uint for AVRs for now. If they work fine on STM32 and/or Linux it should be enough...

When this is done you can finally critique my additions.

@Maju-Ketchup Maju-Ketchup force-pushed the feature/DecaWaveDWM1000Driver branch from 097cc98 to 45ea976 Compare June 4, 2018 07:45
@rleh rleh force-pushed the feature/DecaWaveDWM1000Driver branch from 45ea976 to 935ca59 Compare June 9, 2018 20:39
@rleh
Copy link
Member

rleh commented Jun 9, 2018

I've split @Maju-Ketchup commits to get a better overview.

And replaced execfile() by +exec(compile(open(xpccpath + '/scons/SConstruct', "rb").read(), xpccpath + '/scons/SConstruct', 'exec')) in some SConstruct files.

Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Some examples are double in examples/stm32f4_discovery/ and examples/generic/, but differ slightly.

No file from the examples has a copyright header.

{
LedBlue::set();

//--------------------------------------- Start sending init ---------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessarily long line.
(Not only here)

Copy link
Member

@rleh rleh left a comment

Choose a reason for hiding this comment

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

Should positioning/ be located in math/? @salkinium

@@ -0,0 +1,17 @@
// coding: utf-8
/* Copyright (c) 2018, Roboterclub Aachen e.V.
Copy link
Member

Choose a reason for hiding this comment

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

Nope.

@@ -0,0 +1,199 @@
/**
* Copyright (c) 2018, Marten Junga (Github.com/Maju-Ketchup)
Copy link
Member

Choose a reason for hiding this comment

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

👍

1,anchor0[1]*anchor0[1],anchor1[1]*anchor1[1],anchor2[1]*anchor2[1],
1,anchor0[2]*anchor0[2],anchor1[2]*anchor1[2],anchor2[2]*anchor2[2]};

//Matrix<floatunit, 3,4> A0(a0);
Copy link
Member

Choose a reason for hiding this comment

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

?

struct multilateration
{
public:
/*!
Copy link
Member

Choose a reason for hiding this comment

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

Indentation from here to L140 looks funky 😉

private:


/**
Copy link
Member

Choose a reason for hiding this comment

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

Empty documentation comments have no advantage

buffer[0] = buffer[0];
return true;}
static bool sendAt(int length,uint16_t buffer[], uint32_t time) {
int a = length;
Copy link
Member

Choose a reason for hiding this comment

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

What is the intention?

Maybe:

(void) length;
(void) buffer;
(void) time;

@@ -0,0 +1,21 @@
// coding: utf-8
/* Copyright (c) 2009, Roboterclub Aachen e.V.
Copy link
Member

Choose a reason for hiding this comment

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

Nope.

*
*/

#ifndef XPCC_FRAME802154_CPP
Copy link
Member

Choose a reason for hiding this comment

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

Remove L15-L18

struct frame802154{
public:
enum
Header : uint8_t
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

private:
void setbeginvalues();

// void setbeginvalues();
Copy link
Member

Choose a reason for hiding this comment

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

Delete

Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

Should positioning/ be located in math/?

Yes in xpcc/math/positioning with the top-level header being xpcc/math/positioning.hpp.

* license. See the file `LICENSE` for the full license governing this code.
*
*
* The headder contains the class implementation of the IEEE standart 802.15.4-2011 Frame
Copy link
Member

Choose a reason for hiding this comment

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

header

@@ -0,0 +1,1341 @@
/*<------------------------------------------------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

Should this file be located in ext/? @salkinium

Copy link
Member

@salkinium salkinium Jun 9, 2018

Choose a reason for hiding this comment

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

Yes, anything that's not of our original creation should go to ext, this would go into ext/decawave/deca_regs.hpp?

What license is this file? "All rights reserved." means your not allowed to do anything with it. What file did you adapt this from?

Edit: This file is an original creation not by Decawave.


#include "dw1000.hpp"

//--------------------------------------------------------------XPCC-Functions-Puplic------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

No need for the dashes

//-----------------------------------------


//------------------------------------------------------- CLASS BEGIN -----------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

...

namespace xpcc
{
//-----Without those at this place there will be unfixable bugs (cannot put them into params.hpp) - if anyone knows why pls change
//-----------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

...

#include "./params.hpp"
namespace xpcc
{
//-----Without those at this place there will be unfixable bugs (cannot put them into params.hpp) - if anyone knows why pls change
Copy link
Member

Choose a reason for hiding this comment

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

?



//Lot of Parts copied out of the DWM1000 API added some 'easier to use functions' *mja*
//"Don't include this file directly, use 'dw1000.hpp' instead!"
Copy link
Member

Choose a reason for hiding this comment

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

#ifndef _DW1000_
#error "Don't include this file directly, use 'dw1000.hpp' instead!"
#endif

#include dw1000.hpp // optional, some IDEs need this for better code highlighting or completition


template < typename Spi, typename Cs, typename Reset, typename Irq >
uint32_t
xpcc::Dw1000< Spi, Cs, Reset, Irq >::TX_ANT_DLY = 16436;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this magic value come from?


template < typename Spi, typename Cs, typename Reset, typename Irq >
xpcc::dw1000::IRQreason
xpcc::Dw1000< Spi, Cs, Reset, Irq >::getIRQReason() // TODO
Copy link
Member

Choose a reason for hiding this comment

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

TODO?

rxreset();
}

//--------------------------------------------------------DWM1000-API----------------------------------------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

see above

int
xpcc::Dw1000< Spi, Cs, Reset, Irq >::starttx(TX_Mode mode)
{
/*int retval = SUCCESS ;
Copy link
Member

Choose a reason for hiding this comment

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

Please delete

@@ -0,0 +1,1341 @@
/*<------------------------------------------------------------------------------------------------------------------
Copy link
Member

@salkinium salkinium Jun 9, 2018

Choose a reason for hiding this comment

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

Yes, anything that's not of our original creation should go to ext, this would go into ext/decawave/deca_regs.hpp?

What license is this file? "All rights reserved." means your not allowed to do anything with it. What file did you adapt this from?

Edit: This file is an original creation not by Decawave.

#ifndef _DECA_REGS_H_
#define _DECA_REGS_H_

#include <xpcc/architecture/platform.hpp>
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't depend on platform, only on stdint.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My predecessor took this file from the original Decawave API, which is a horrible C code

static constexpr uint32_t DEV_ID_REV_MASK =0x0000000FUL; /*<Revision */
static constexpr uint32_t DEV_ID_VER_MASK =0x000000F0UL; /*<Version */
static constexpr uint32_t DEV_ID_MODEL_MASK =0x0000FF00UL; /*<The MODEL identifies the device. The DW1000 is device type =0x01 */
static constexpr uint32_t DEV_ID_RIDTAG_MASK =0xFFFF0000UL; /*<Register Identification Tag =0xDECA */
Copy link
Member

Choose a reason for hiding this comment

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

Was the original file done with a lot of #defines? And if so why did you translate this to C++ constexpr?

Copy link
Contributor Author

@Maju-Ketchup Maju-Ketchup Jun 11, 2018

Choose a reason for hiding this comment

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

Historical reasons my predecessor did that. I don't know why

  • edit - I think the reasoning about it was to be focused on types, that an error using the wrong kind of type could be found faster. But I'm just guessing here.
    I also changed some (at least 2) of the registers vs the original file because they were wrong...

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then this is original work, since you cannot copyright the data, only the formatting. Can you put your predecessor and yourself as copyright owners (BSD-licensed)?

I would then not put this into ext/, since we also don't put the register definitions of our sensor devices from modm/driver into ext/ either.

@salkinium
Copy link
Member

salkinium commented Jun 24, 2018

FYI: I've just put up our deprecation notice for xpcc, as we are currently moving on to modm as its successor. I will be porting all new code to modm for the next 6 months, but in Jan 2019 I will be archiving this repository and then no more contribution can make it into xpcc.

So… no pressure, but just keep it in mind for your time planning.

@salkinium
Copy link
Member

I already asked you this before in #318 (comment), but are you/your employer still ok with dual licensing this contribution as BSD for xpcc and MPLv2 for modm?

@salkinium salkinium removed this from the 2018q2 milestone Jun 28, 2018
@salkinium
Copy link
Member

@Maju-Ketchup any update on this? We still want this in xpcc, tell us how we can help you with this.

@Maju-Ketchup
Copy link
Contributor Author

Hi there,

sorry for my long absence but i had no time due to exams and a high workload.
My idea for this is that I split my work into 3 pieces as @rleh commented.

-The Decawave DW1000 driver
-The MAC-Frame of IEEE 802.15.4
-The trilateration and ranging algorithms/framework

The last part is highly work in progress and throws some issues.
So i would leave it out for now.

my workprocess would be the following for the next step:

-Close this merge request.
-Add the 802.15.4 Frame to xpcc
-Add the Decawave driver to xpcc
-Add the Ranging Framework to xpcc/modm
-Add the examples

I will add feature branches for everything during the next week.
Or is there something else on your mind?

@salkinium
We are totally okay at my workplace to change the license to MPLv2 and include it into modm.

@salkinium
Copy link
Member

Or is there something else on your mind?

No, that sounds perfectly fine for me. This doesn't need to be perfect, just good enought that it'll pass the examples/tests, so I can port it to modm with reasonable assurance that I didn't break anything.
Although the API changes will only be cosmetic, and not functional.

@salkinium
Copy link
Member

As announced, I'm going to archive the xpcc repository soon. You fork will not be influenced, you can still use it as long as you need to. Looking at your code, I'm fairly positive that it can be ported to modm with only a few changes (and a healthy dose of s/xpcc/modm), if you still want to do that.

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

Successfully merging this pull request may close these issues.

3 participants