jgromes / RadioLib

Universal wireless communication library for embedded devices
https://jgromes.github.io/RadioLib/
MIT License
1.49k stars 376 forks source link

SX126x.h setPacketParamsFSK() defaults to WHITENING_ON #45

Closed mmrein closed 4 years ago

mmrein commented 4 years ago

This can be considered bug or feature, so I'll leave it as a general question.

Is there a reason why whitening on SX126x defaults to ON? This got me stuck for few days until I found it, now I'm finally receiving some data that starts to make sense (I'm missing first byte of data, but that may be another issue).

Can we add a method that sets the whitening? Something like:

int16_t setWhitening(bool whiteningEnabled);

I'm also not sure about other default values defined by:

int16_t setPacketParamsFSK(uint16_t preambleLength, uint8_t crcType, uint8_t syncWordLength, uint8_t addrComp, uint8_t payloadLength = 0xFF, uint8_t packetType = SX126X_GFSK_PACKET_VARIABLE, uint8_t preambleDetectorLength = SX126X_GFSK_PREAMBLE_DETECT_16, uint8_t whitening = SX126X_GFSK_WHITENING_ON);

Doesn't it work the way that whenever the

setPacketParamsFSK(_preambleLengthFSK, _crcTypeFSK, _syncWordLength, _addrComp);

is used in code (setCRC(), disableAddressFiltering(), setSyncWord() etc.), it will actually use those defaults?

EDIT: And the missing first byte is because of SX126X_GFSK_PACKET_VARIABLE. So I guess we should think about how to implement these settings too.

jgromes commented 4 years ago

If you poke around in the code enough (mainly in the SX127x driver), you'll see that are some settings of the radio that are not exposed to the public API. The reason is that most users won't ever need them, so when there's such a parameter, I set it to the default value from datasheet.

Is there a reason why whitening on SX126x defaults to ON?

There's no particular reason, my usual use-case was sending data between the same radios, so the whitener was never an issue (because it was configured the same way on both ends). If you want to add a new method that sets it, feel free to open a PR - and while we're at it, we could allow user to set whitener initial value. e.g.

int16_t setWhitening(bool enabled, uint16_t initial = 0x0100);

0x0100 is the default whitener initial value. According to Semtech, this is also the initial value on SX127x series, but I never managed to get two talking together via FSK, so it might be a complete lie.

I'm also not sure about other default values defined by:

Not entirely sure what you mean there - could you elaborate?

SX126X_GFSK_PACKET_VARIABLE as packet type is required by the current API (see SX126x datasheet section 6.2.3) - implementing fixed-length packet transmission would most likely mean adding new transmission methods. There was an attempt to implement non-variable length packets in LoRaLib (see https://github.com/jgromes/LoRaLib/pull/66), but it never went anywhere.

mmrein commented 4 years ago

Thank you for the explanation.

Not entirely sure what you mean there - could you elaborate?

I was not sure if the default values are always used but you explained it celarly by this:

The reason is that most users won't ever need them, so when there's such a parameter, I set it to the default value from datasheet.

And this explains why I was not able to receive meaningful data on SX1261 while I was transmitting from SX1276:

my usual use-case was sending data between the same radios, so the whitener was never an issue

I'm not sure I could incorporate the fixed packet length transmission, let alone the large packet handling, it just explains for me why the first byte was missing.

I guess I could try and do something about the whitening setup.

I'm also thinking about preamble detector length which I got confused with preambleLength used in beginFSK() (which is TX preamble length). The default 16 bits for Rx is just ok so I don't really need to change this, but if I'm going to add the whitening setup, it may not be much more work to add this too, what do you think?

jgromes commented 4 years ago

Sure, if you think you have a use for it, go ahead and add it. I'm not entirely sure whether to add preamble detector length to the existing setPreambleLength method (the usage of the two parameters is somewhat linked afterall), or add a new method. SX126x only allows setting detector length in multiples of 8, so I think we can have the detector length parameter in bytes.

Also, if you decide to make these changes, please leave them as completely optional - that is, the default should remain whitener on + 16 bit detector. They also shouldn't be integrated into the beginFSK() method.

mmrein commented 4 years ago

Sure, if you think you have a use for it, go ahead and add it. I'm not entirely sure whether to add preamble detector length to the existing setPreambleLength method (the usage of the two parameters is somewhat linked afterall), or add a new method. SX126x only allows setting detector length in multiples of 8, so I think we can have the detector length parameter in bytes.

If so, I would make separate method because you can have (and is seems to be better/recomended) different TX and Rx preamble lengths. I'm actually using it - I have 32bit Tx preamble, but I only use 16bit on detector.

I have tried other lengths on Rx side just now and result is that 16bit is only useful value. With 8bit it kinda works, but lot of wrong packets were received. 16bits is fine. 24 and 32 bits doesn't really receive any good packet.

That means the possibility of setting PreambleDetectorLength is not really needed and leaves me only with possibility to setup whitening.

Yes, I agree it shouldn't be in beginFSK(). I'll try to present some solution when I get to it and let you know here or over PR.

jgromes commented 4 years ago

Sorry, what I meant was something along the lines of

int16_t setPreambleLength(uint16_t preambleLength, uint8_t preambleDetectorLength);

I think you'd have to use longer preamble if detector is set to 24 or 32 bit. Haven't tried though.

mmrein commented 4 years ago

Oh, ok. Then it can default to preambleDetectorLength = SX126X_GFSK_PREAMBLE_DETECT_16 so it will not break anything and you can still use setPreambleLength(32) for example, if I'm not mistaken.

I'm not sure if it wouldn't be confusing with preambleLength in beginFSK() though.

I think you'd have to use longer preamble if detector is set to 24 or 32 bit.

It looks so.

mmrein commented 4 years ago

So, the whitening setup works. Sort of... Enable/disable works well, but there is an issue when I try to change whitening value, ie. writing to registers, like so for example:

// the 0x0100 is default value (pg. 46 or 91 of datasheet v1.2).
uint16_t initial = 0x0100;

// write initial whitening value
uint8_t data[2];
data[0] = (uint8_t)((initial >> 8) & 0xFF);
data[1] = (uint8_t)(initial & 0xFF);
state = writeRegister(SX126X_REG_WHITENING_INITIAL_MSB, data, 2);

I can not get any Rx. Init and setup goes without errors, radio goes to the Rx mode, but no message can be received.

There is a note on page 65 of datasheet v1.2:

obrazek

Which to me seems kinda weird, because how can I not change undefined value? Should I read the register first? How come it is undefined when pg. 46 or 91 says the default value is 0x01? "The whitening is based around the 9-bit LFSR polynomial", so changing those 7 MSBs should not even make any difference...

Anyways, I tried to write the SX126X_REG_WHITENING_INITIAL_LSB register only (not touching the MSB) and it works, radio does receive packets again. Writing 0x00 has same output as default value, writing different value changes the decoded data.

I guess I can try to read the MSB register prior to writing it, or else I donť understand why it should not work.

EDIT: ok, that really does the trick.

jgromes commented 4 years ago

Welcome to the insane world of Semtech's docs, where no two statements are the same and everything is as confusing as possible :)

0x100 is probably the actual initial value, since that number is exactly 9 bits long. However, that is saved in two 8-bit registers, and there is no way to tell what might be saved in the top 7 bits of whitening MSG register - hence, reading the register and then writing to it is the only safe way to go (as you correctly figured out). I guess the appropriate section of table 12-1 should read:

| Whitening initial value MSB | 0x06B8 | 0bXXXX XXX1 | | Whitening initial value LSB | 0x06B9 | 0b0000 0000 |

The most frustrating part of SX126x is the huge amount of undocumented registers ...

mmrein commented 4 years ago

I see. There are some undocumented fetures on Sx127x too, but that's only a small part. On SX126x only a small part is documented :)

So I'm gonna PR the whitening for SX126x. I'm still not convinced to do the preambleDetectorLength mainly because I have other higher poriority things to do, so I guess we can close this issue for now.

jgromes commented 4 years ago

Merged in #46