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

FW-861 Enable FSK-FEC 25kbps for TSCH link using 120ms slot duration #492

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

Conversation

minarady1
Copy link
Contributor

  • changed slot duration to 120 ms
  • disabled channel hopping
  • adapted EB portion to slot duration: use 4 for 120 ms or 10 otherwise.
  • adapted radio watchdog parameters to slot duration

+ changed slot duration to 120 ms
+ disabled channel hopping
+ adapted EB portion to slot duration: use 4 for 120 ms or 10 otherwise.
+ adapted radio watchdog parameters to slot duration
Copy link
Member

@changtengfei changtengfei left a comment

Choose a reason for hiding this comment

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

Just added some comments. I saw you created another timing for FSK which is good!
Ideally the new timing for each FSK is radio modulation specific, which should be not in IEEE802154E.h file. (This work is done in Jonathan's branch) . But we may try to immigrate to jonathan's branch later.

Have you tested this PR? are you able to run a network with this setting?
Try to write something about how you tested and what suppose to happen for others to reproduce.

@@ -116,7 +116,7 @@ void ieee154e_init(void) {
memset(&ieee154e_dbg,0,sizeof(ieee154e_dbg_t));

// set singleChannel to 0 to enable channel hopping.
ieee154e_vars.singleChannel = 0;
ieee154e_vars.singleChannel = 11;
Copy link
Member

Choose a reason for hiding this comment

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

by default, channel hopping is enabled, please reverse this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, reveresed it.

Copy link
Member

Choose a reason for hiding this comment

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

resolve here

#define MAXKAPERIOD 1000 // in slots: 1500@20ms per slot -> ~30 seconds. Max value used by adaptive synchronization.
#define DESYNCTIMEOUT 1750 // in slots: 1750@20ms per slot -> ~35 seconds. A larger DESYNCTIMEOUT is needed if using a larger KATIMEOUT.
#define LIMITLARGETIMECORRECTION 5 // threshold number of ticks to declare a timeCorrection "large"
#define LENGTH_IEEE154_MAX 128 // max length of a valid radio packet
#define DUTY_CYCLE_WINDOW_LIMIT (0xFFFFFFFF>>1) // limit of the dutycycle window
#define SERIALINHIBITGUARD 32 // 32@32kHz ~ 1ms

// set EB on minimal cell for 1/EB_PORTION portion
Copy link
Member

Choose a reason for hiding this comment

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

You can just change EB_PORTION value when necessary at local.
No need to commit this changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, removed it but added a comment before this configuration to explain that it is connected to the change in slot duration. Otherwise synchronizations become nearly impossible for longer slot duration

Copy link
Member

@changtengfei changtengfei left a comment

Choose a reason for hiding this comment

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

I left some comments for the PR, could you check and resolve them?

@@ -116,7 +116,7 @@ void ieee154e_init(void) {
memset(&ieee154e_dbg,0,sizeof(ieee154e_dbg_t));

// set singleChannel to 0 to enable channel hopping.
ieee154e_vars.singleChannel = 0;
ieee154e_vars.singleChannel = 11;
Copy link
Member

Choose a reason for hiding this comment

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

resolve here

Must be set to lower values for slower modulations. Otherwise, impossible to synchronize.
Default setting is 10 for 20 ms slot duration and 4 for 120 ms slot duration
*/
#define EB_PORTION 4
Copy link
Member

Choose a reason for hiding this comment

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

This variable is configurable, leave it as 10 by default. It influence the synchronization time, but still be able to synchronize with larger value.

@@ -42,7 +42,7 @@ int mote_main(void) {
board_init();
scheduler_init();
openstack_init();
if (idmanager_getMyID(ADDR_64B)->addr_64b[7]==0x16) {
if (idmanager_getMyID(ADDR_64B)->addr_64b[7]==0x6b) {
Copy link
Member

Choose a reason for hiding this comment

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

should not commit this change

@@ -109,7 +109,7 @@ void iphc_init(void) {
macpong_vars.timerId = opentimers_create(TIMER_GENERAL_PURPOSE, TASKPRIO_IPHC);
opentimers_scheduleIn(
macpong_vars.timerId, // timerId
1000, // duration
1000, // duration: longer duration is preferable for slower connections to avoid buffer overflow. 4000 was used for FSK at 25kbps wnad it was fine
Copy link
Member

Choose a reason for hiding this comment

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

This timer is canceled when the a packet is generated. It should not be influenced by the modulation.

@minarady1
Copy link
Contributor Author

Just fixed them, sorry forgot to undo the changes.

@changtengfei
Copy link
Member

changtengfei commented Dec 3, 2019

@minarady1 two things:

Is this PR the one we tested last week?
Could you confirm you are using a 160ms, but not a 120ms, since I saw comments in the code saying 120ms.
Other than that, I think this PR is good to merge

- Used Atmel header file at86rf215.h with register configurations from the SubGhz Project>> OQPSK R3 still does not work and Bitrates doen't make sense
- Updated radio.h to use LENGTH_CRC 2 instead of 4
- Updated few spots in IEEE802154E.c to refer to the LENGTH_CRC variable instead of using fixed numerical value.
- Updated EB portion in IEEE802154E.h to 4 instead of 10 for faster sync time, for the sake of experimentation at least.
- Updated SLOTFRAME_LENGTH to be 41 instead of 101 for faster sync for experimentation purposes
- Added buffer_counter as a global variable in openqueue.c to allow tracing of buffer size using live watch on the JTAG to track congestion and buffer overflow in the packet queue in a live manner
- Updated the openmote-b-24ghz/cc2538rf.h interface to include definitions for MIN/MAX/RECOMMENDED TX_POWER
- Created radio_setConfig function as part of the radio driver prototype and implemented it for cc2538 2.4 ghz radio in radio.c
- Added rfConfig parameter for the neighborRow_t in opendefs.h to describe neighbor's desired rfConfig

- Updated IEEE802154E.c and IEEE802154E.h to include adaptive radio configurations:
- Added, DefaultRFConfig, CurrentRFConfig paramters to ieee154e_vars in IEEE802154E.c
- Added UseDefaultRFConfig flag to ieee154e_vars in IEEE802154E.c

- Updated neighbors.c and neighbors.h to allow adaptive RF neighbor Configuration:
- neighbors_getRFConfig
- defined link quality thresholds and neighbor quality thresholds
@@ -965,9 +969,16 @@ port_INLINE void activity_ti1ORri1(void) {
if (packetfunctions_isBroadcastMulticast(&neighbor)==FALSE){

// look for a unicast packet to send
ieee154e_vars.dataToSend = openqueue_macGetUnicastPakcet(&neighbor); //**Packet not Pakcet
Copy link
Member

Choose a reason for hiding this comment

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

This line is written twice.

@@ -121,6 +121,9 @@ void ieee154e_init(void) {
ieee154e_vars.isSecurityEnabled = FALSE;
ieee154e_vars.slotDuration = TsSlotDuration;
ieee154e_vars.numOfSleepSlots = 1;
ieee154e_vars.DefaultRFConfig = 2;
ieee154e_vars.CurrentRFConfig = 2;
ieee154e_vars.UseDefaultRFConfig = 1;
Copy link
Member

@changtengfei changtengfei Feb 21, 2020

Choose a reason for hiding this comment

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

is this variable an integer?

@@ -1276,7 +1295,8 @@ port_INLINE void activity_ti5(PORT_TIMER_WIDTH capturedTime) {
// 3. set capture for receiving SFD and packet receiving done
sctimer_setCapture(ACTION_RX_SFD_DONE);
sctimer_setCapture(ACTION_RX_DONE);

radio_setFrequency(ieee154e_vars.freq);
Copy link
Member

Choose a reason for hiding this comment

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

written twice


//check if the radio needs to use the default RF or a custom RF config and configure accordingly.
if (ieee154e_vars.UseDefaultRFConfig)
{
Copy link
Member

Choose a reason for hiding this comment

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

Move this bracket right after the if condition.

//check if the radio needs to use the default RF or a custom RF config and configure accordingly.
if (ieee154e_vars.UseDefaultRFConfig)
{
radio_setConfig (ieee154e_vars.DefaultRFConfig);
Copy link
Member

Choose a reason for hiding this comment

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

IEEE802154e.c file is a shared file for all OpenWSN port.
If this function is called here, you need implement this function at each of the OpenWSN platforms.

@@ -1276,7 +1295,8 @@ port_INLINE void activity_ti5(PORT_TIMER_WIDTH capturedTime) {
// 3. set capture for receiving SFD and packet receiving done
sctimer_setCapture(ACTION_RX_SFD_DONE);
sctimer_setCapture(ACTION_RX_DONE);

radio_setFrequency(ieee154e_vars.freq);
radio_setConfig (ieee154e_vars.CurrentRFConfig);
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -1915,6 +1935,7 @@ port_INLINE void activity_ri5(PORT_TIMER_WIDTH capturedTime) {

// configure the radio for that frequency
radio_setFrequency(ieee154e_vars.freq);
radio_setConfig (ieee154e_vars.CurrentRFConfig);
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -121,6 +121,9 @@ void ieee154e_init(void) {
ieee154e_vars.isSecurityEnabled = FALSE;
ieee154e_vars.slotDuration = TsSlotDuration;
ieee154e_vars.numOfSleepSlots = 1;
ieee154e_vars.DefaultRFConfig = 2;
Copy link
Member

@changtengfei changtengfei Feb 21, 2020

Choose a reason for hiding this comment

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

Give a meaningful name of the value of this variable, i.e. use enumerate type.

@@ -121,6 +121,9 @@ void ieee154e_init(void) {
ieee154e_vars.isSecurityEnabled = FALSE;
ieee154e_vars.slotDuration = TsSlotDuration;
ieee154e_vars.numOfSleepSlots = 1;
ieee154e_vars.DefaultRFConfig = 2;
ieee154e_vars.CurrentRFConfig = 2;
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -255,7 +276,9 @@ typedef struct {
PORT_TIMER_WIDTH radioOnInit; // when within the slot the radio turns on
PORT_TIMER_WIDTH radioOnTics; // how many tics within the slot the radio is on
bool radioOnThisSlot; // to control if the radio has been turned on in a slot.

bool UseDefaultRFConfig; // tells if we are in a state to use the default RF config (e.g. shared state) or use a custom RF config (e.g. an active negotiated slot).
uint8_t DefaultRFConfig; // defines the RF config to use by default (such as on shared slots, and perhaps for retransmissions later?). This currenly represents only level of Tx power. Later it should be a struct representing heterogeneous configuration.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this variable is related to slot type?
If this is for TxPower, just use variable name like txpower.
However, tx power should not be part of the IEEE802.15.4


// update closeNeighbor, switchQualityCounter
if (neighbors_vars.neighbors[i].rssi>GOODLINKMINRSSI){
neighbors_vars.neighbors[i].rfConfig=1;
Copy link
Member

Choose a reason for hiding this comment

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

Please replace 1, 2, 3 by something readable.

if (neighbors_vars.neighbors[i].rssi>BADLINKMAXRSSI){
neighbors_vars.neighbors[i].rfConfig=2;
}else {
neighbors_vars.neighbors[i].rfConfig=3;
Copy link
Member

Choose a reason for hiding this comment

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

alignment.

#define BADLINKMAXRSSI -40 //dBm
#define GOODLINKMINRSSI -30 //dBm

#define CLOSE_NEIGHBOR 1
Copy link
Member

Choose a reason for hiding this comment

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

Those definitions are not related what this PR is try to do.

@@ -15,6 +15,14 @@
#define MAXPREFERENCE 2
#define BADNEIGHBORMAXRSSI -70 //dBm
#define GOODNEIGHBORMINRSSI -80 //dBm

#define BADLINKMAXRSSI -40 //dBm
Copy link
Member

@changtengfei changtengfei Feb 21, 2020

Choose a reason for hiding this comment

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

should not touch this change.

@@ -15,6 +15,14 @@
#define MAXPREFERENCE 2
#define BADNEIGHBORMAXRSSI -70 //dBm
#define GOODNEIGHBORMINRSSI -80 //dBm

#define BADLINKMAXRSSI -40 //dBm
#define GOODLINKMINRSSI -30 //dBm
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -17,7 +17,7 @@

The superframe reappears over time and can be arbitrarily long.
*/
#define SLOTFRAME_LENGTH 101 //should be 101
#define SLOTFRAME_LENGTH 41 //should be 101
Copy link
Member

Choose a reason for hiding this comment

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

This change needs to be reverted

@@ -20,7 +20,7 @@ openqueue_vars_t openqueue_vars;
void openqueue_reset_entry(OpenQueueEntry_t* entry);

//=========================== public ==========================================

uint8_t buffer_counter =0; //for debugging purposes: used to track packet queue size using JTAG live watch.
Copy link
Member

@changtengfei changtengfei Feb 21, 2020

Choose a reason for hiding this comment

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

Should not be commited

@changtengfei
Copy link
Member

@minarady1 I have left lots comments. It seems this PR is actually not ready yet. There are lots of things that not related what the title of this PR. I will leave this PR for now. And resume to review when the comments are resolve. Sounds good?

@minarady1
Copy link
Contributor Author

minarady1 commented Mar 3, 2020

Thanks @changtengfei , sorry I was not pushing to git correctly and I pushed irrelevant changes to you. I will make a new push.

@changtengfei
Copy link
Member

@minarady1 do you still want to keep this PR? As the timing for FSK will be in the PR FW-850.

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

Successfully merging this pull request may close these issues.

2 participants