search

thelsing / knx

knx stack (TP, IP and RF) for arduino and linux, Can be configured with ETS
GNU General Public License v3.0
257 stars 91 forks source link

Add support for CC1310 #92

Closed nanosonde closed 3 years ago

nanosonde commented 3 years ago

As already mentioned here it would be nice to support the CC1310 as new platform in the stack.

@mjm987 You stated here that you have started to work on this. As I have also started to work in this it would be nice to join efforts. Still my current status is that I only have a modified simple EasyLink example to receive KNX-RF frames. What do have already? Are you able to do TX yet?

As a kind of pre-work I have added platformio support for the CC1310: https://github.com/nanosonde/platform-ticc13x0 So I can compile the stack with Energia (a.k.a. Arduino) core.

mjm987 commented 3 years ago

My CC1310 port already runs KNX-RF pretty well but the software is far from clean. As build environment I use CCS (TI Code Composer Studio) using GCC Toolchain (I don't know PlatformIO).

The main adaption was done in "cc1310_platform." and "rf_physical_cc1310."

Following should be solved:

And the nice side:

nanosonde commented 3 years ago

Very cool!

Do you know this document? https://www.ti.com/lit/pdf/swra512 It covers wireless M-BUS S-Mode which is basically the RF stuff required for KNX-RF.

My settings: 

// TI-RTOS RF Mode Object
RF_Mode RF_prop =
{
    .rfMode = RF_MODE_PROPRIETARY_SUB_1,
    .cpePatchFxn = &rf_patch_cpe_wmbus_smode,
    .mcePatchFxn = &rf_patch_mce_wmbus_smode,
    .rfePatchFxn = &rf_patch_rfe_wmbus_smode,
};

// Overrides for CMD_PROP_RADIO_DIV_SETUP
static uint32_t pOverrides[] =
{
    // PHY: Run the MCE and RFE patches
    MCE_RFE_OVERRIDE(1,0,0,1,0,0),
    // override_synth_prop_863_930_div5.xml
    // Synth: Set recommended RTRIM to 7
    HW_REG_OVERRIDE(0x4038,0x0037),
    // Synth: Set Fref to 4 MHz
    (uint32_t)0x000684A3,
    // Synth: Configure fine calibration setting
    HW_REG_OVERRIDE(0x4020,0x7F00),
    // Synth: Configure fine calibration setting
    HW_REG_OVERRIDE(0x4064,0x0040),
    // Synth: Configure fine calibration setting
    (uint32_t)0xB1070503,
    // Synth: Configure fine calibration setting
    (uint32_t)0x05330523,
    // Synth: Set loop bandwidth after lock to 20 kHz
    (uint32_t)0x0A480583,
    // Synth: Set loop bandwidth after lock to 20 kHz
    (uint32_t)0x7AB80603,
    // Synth: Configure VCO LDO (in ADI1, set VCOLDOCFG=0x9F to use voltage input reference)
    ADI_REG_OVERRIDE(1,4,0x9F),
    // Synth: Configure synth LDO (in ADI1, set SLDOCTL0.COMP_CAP=1)
    ADI_HALFREG_OVERRIDE(1,7,0x4,0x4),
    // Synth: Use 24 MHz XOSC as synth clock, enable extra PLL filtering
    (uint32_t)0x02010403,
    // Synth: Configure extra PLL filtering
    (uint32_t)0x00108463,
    // Synth: Increase synth programming timeout (0x04B0 RAT ticks = 300 us)
    (uint32_t)0x04B00243,
    // override_synth_disable_bias_div5.xml
    // Synth: Set divider bias to disabled
    HW32_ARRAY_OVERRIDE(0x405C,1),
    // Synth: Set divider bias to disabled (specific for loDivider=5)
    (uint32_t)0x18000200,
    // override_phy_rx_aaf_bw_0xd.xml
    // Rx: Set anti-aliasing filter bandwidth to 0xD (in ADI0, set IFAMPCTL3[7:4]=0xD)
    ADI_HALFREG_OVERRIDE(0,61,0xF,0xD),
    // override_phy_gfsk_rx.xml
    // Rx: Set LNA bias current trim offset to 3
    (uint32_t)0x00038883,
    // Rx: Freeze RSSI on sync found event
    HW_REG_OVERRIDE(0x6084,0x35F1),
    // Tx: Configure PA ramping setting (0x61). Rx: Set AGC reference level to 0x1F
    HW_REG_OVERRIDE(0x6088,0x611F),
    // Tx: Configure PA ramping setting and setting AGC settle wait = 21 samples
    HW_REG_OVERRIDE(0x608C,0x8112),
    // Rx: Set RSSI offset to adjust reported RSSI by +7 dB
    (uint32_t)0x00F988A3,
    // TX power override
    // Tx: Set PA trim to max (in ADI0, set PACTL0=0xF8)
    ADI_REG_OVERRIDE(0,12,0xF8),
    // Set AGC win size = 7 samples
    HW_REG_OVERRIDE(0x6064,0x1306),
    (uint32_t)0xFFFFFFFF,
};

// CMD_PROP_RADIO_DIV_SETUP
rfc_CMD_PROP_RADIO_DIV_SETUP_t RF_cmdPropRadioDivSetup =
{
    .commandNo = 0x3807,
    .status = 0x0000,
    .pNextOp = 0, // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
    .startTime = 0x00000000,
    .startTrigger.triggerType = 0x0,
    .startTrigger.bEnaCmd = 0x0,
    .startTrigger.triggerNo = 0x0,
    .startTrigger.pastTrig = 0x0,
    .condition.rule = 0x1,
    .condition.nSkip = 0x0,
    .modulation.modType = 0x0,
    .modulation.deviation = 0xC8,
    .symbolRate.preScale = 0xF,
    .symbolRate.rateWord = 0x53E3,
    .rxBw = 0x27, // S2-mode (for S1-mode, rxBw = 0x29)
    .preamConf.nPreamBytes = 0x3,
    .preamConf.preamMode = 0x0,
    .formatConf.nSwBits = 0x18,
    .formatConf.bBitReversal = 0x0,
    .formatConf.bMsbFirst = 0x1,
    .formatConf.fecMode = 0x0a, // manchester coding
    .formatConf.whitenMode = 0x0,
    .config.frontEndMode = 0x0,
    .config.biasMode = 0x1,
    .config.analogCfgMode = 0x0,
    .config.bNoFsPowerUp = 0x0,
    .txPower = 0xA73F,
    .pRegOverride = pOverrides,
    .centerFreq = 0x0364,
    .intFreq = 0x8000,
    .loDivider = 0x05,
};

// CMD_FS
rfc_CMD_FS_t RF_cmdFs =
{
    .commandNo = 0x0803,
    .status = 0x0000,
    .pNextOp = 0, // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
    .startTime = 0x00000000,
    .startTrigger.triggerType = 0x0,
    .startTrigger.bEnaCmd = 0x0,
    .startTrigger.triggerNo = 0x0,
    .startTrigger.pastTrig = 0x0,
    .condition.rule = 0x1,
    .condition.nSkip = 0x0,
    .frequency = 0x0364,
    .fractFreq = 0x4CCD,
    .synthConf.bTxMode = 0x0,
    .synthConf.refFreq = 0x0,
    .__dummy0 = 0x00,
    .__dummy1 = 0x00,
    .__dummy2 = 0x00,
    .__dummy3 = 0x0000,
};

For the rfc_CMD_PROP_RX_ADV_t I use the following setting:

  .syncWord0 = 0x547696,

I also modified it to use the fixed value 0x44 as address so that I have an additiona lcheck in HW before I really get the packet. I use the unlimited packet receive mode with DATA_ENTRY_TYPE_PARTIAL. After receiving the first few bytes I set the packet length using CMD_PROP_SET_LEN to receive the rest of the packet. Unfortunately, CRC seems not to be usable because of multiple CRCs.

Do you have multiple threads or just one? Have a look here: https://github.com/nanosonde/platform-ticc13x0/blob/master/examples/arduino-blink/src/main.cpp

Yes, for rfphysical we need a solution. In my option they should be renamed to rfdriver and should somehow be part of the platform instead of the rf_datalink_layer. The platform should then offer some rf_driver interface to the rf_datalink_layer. @thelsing What do you think?

And second yes :), it would be really good to modify the stack so that it better supports standby. We need a solution for all the timers here. If nothing needs to be done, then the stack should sleep instead of busy loop around. For this we need to collect timer timeout values in a central global instance so that we can check in the main loop when to wake up again according to the timer with smallest timeout value. Or something like that. Only wakeup sources would be:

mjm987 commented 3 years ago

No I didn't know that M-BUS S-Mode is basically the same. Thx for the pdf link - very interesting! My SamartRF Setup i did with SmartRF Studio with later manual changes (see the diff to yours below).

More or less I did it same as you did (incl. 24bit Sync!) but your idea to trigger the 0x44 as address is really brilliant! I capture the first fragment till the first CRC so I can verify the CRC and then decided to proceed further or stop receiving.

Currently I use only one task but with more tasks and using semaphores with timeout the looping could be prevented - but but I fear this would need a lot of changes in the knx lib - I think looping is not a big problem with eg. 1ms sleep between loops.

The ldeep sleep and wakeup works already for the timer and gpio should also be possible quite easy.

I will upload what I have to my github repo but first I would like to cleanup at least a bit.

Here the diff between yours (<) and my (>) samrtrf settings:


 0a1,39
> //*********************************************************************************
> // Generated by SmartRF Studio version 2.17.0 (build#228)
> // The applied template is compatible with CC13x0 SDK version 2.10.xx.xx or newer.
> // Device: CC1310 Rev. A (2.0).
> //
> //*********************************************************************************
> 
> 
> //*********************************************************************************
> // Parameter summary
> // RX Address0: 0xAA
> // RX Address1: 0xBB
> // RX Address Mode: No address check
> // Frequency: 868.30000 MHz
> // Data Format: Serial mode disable 
> // Deviation: 60.000 kHz
> // Fixed Packet Length: 20
> // Packet Length Config: Fixed
> // Max Packet Length: 255 
> // Packet Data: 255 
> // RX Filter BW: 196 kHz
> // Symbol Rate: 32.76825 kBaud
> // Sync Word Length: 32 Bits
> // TX Power: 14 dBm
> // Whitening: No whitening 
> 
> // mm: modified for Manchester Decoding
> 
> #include <ti/devices/DeviceFamily.h>
> #include DeviceFamily_constructPath(driverlib/rf_mailbox.h)
> #include DeviceFamily_constructPath(driverlib/rf_common_cmd.h)
> #include DeviceFamily_constructPath(driverlib/rf_prop_cmd.h)
> #include <ti/drivers/rf/RF.h>
> #include DeviceFamily_constructPath(rf_patches/rf_patch_cpe_genfsk.h)
> #include DeviceFamily_constructPath(rf_patches/rf_patch_rfe_genfsk.h)
> #include "smartrf_settings.h"
> 
> 
> 
5,7c44,46
<     .cpePatchFxn = &rf_patch_cpe_wmbus_smode,
<     .mcePatchFxn = &rf_patch_mce_wmbus_smode,
<     .rfePatchFxn = &rf_patch_rfe_wmbus_smode,
---
>     .cpePatchFxn = &rf_patch_cpe_genfsk,
>     .mcePatchFxn = 0,
>     .rfePatchFxn = &rf_patch_rfe_genfsk
11c50
< static uint32_t pOverrides[] =
---
> uint32_t pOverrides[] =
13,14c52,57
<     // PHY: Run the MCE and RFE patches
<     MCE_RFE_OVERRIDE(1,0,0,1,0,0),
---
>     // override_use_patch_prop_genfsk.xml
> #ifndef HW_MANCHESTER_DECODE
>  MCE_RFE_OVERRIDE(0,4,0,1,0,0),
> #else
>  MCE_RFE_OVERRIDE(0,5,0,1,0,0),
> #endif
42,46d84
<     // override_synth_disable_bias_div5.xml
<     // Synth: Set divider bias to disabled
<     HW32_ARRAY_OVERRIDE(0x405C,1),
<     // Synth: Set divider bias to disabled (specific for loDivider=5)
<     (uint32_t)0x18000200,
55,66c93,104
<     // Tx: Configure PA ramping setting (0x61). Rx: Set AGC reference level to 0x1F
<     HW_REG_OVERRIDE(0x6088,0x611F),
<     // Tx: Configure PA ramping setting and setting AGC settle wait = 21 samples
<     HW_REG_OVERRIDE(0x608C,0x8112),
<     // Rx: Set RSSI offset to adjust reported RSSI by +7 dB
<     (uint32_t)0x00F988A3,
<     // TX power override
<     // Tx: Set PA trim to max (in ADI0, set PACTL0=0xF8)
<     ADI_REG_OVERRIDE(0,12,0xF8),
<     // Set AGC win size = 7 samples
<     HW_REG_OVERRIDE(0x6064,0x1306),
<     (uint32_t)0xFFFFFFFF,
---
>     // override_phy_gfsk_pa_ramp_agc_reflevel_0x1a.xml
>     // Tx: Configure PA ramping setting (0x41). Rx: Set AGC reference level to 0x1A.
>     HW_REG_OVERRIDE(0x6088,0x411A),
>     // Tx: Configure PA ramping setting
>     HW_REG_OVERRIDE(0x608C,0x8213),
> 
> #ifdef HW_MANCHESTER_DECODE
>     ////0x00fb88a3, // ??? test with Manchester encoding
>     //HW_REG_OVERRIDE(0x52b4,0x270c), // test with Manchester encoding
>     ////ADI_REG_OVERRIDE(0,12,0xf8),  // ??? test with Manchester encoding
> #endif
>     (uint32_t)0xFFFFFFFF
68a107
> 
69a109
> // Proprietary Mode Radio Setup Command for All Frequency Bands
82,83c122,123
<     .modulation.modType = 0x0,
<     .modulation.deviation = 0xC8,
---
>     .modulation.modType = 0x1,
>     .modulation.deviation = 0xdc, //  55kHz:0xDC, 60kHz:0xF0
86,87c126,128
<     .rxBw = 0x27, // S2-mode (for S1-mode, rxBw = 0x29)
<     .preamConf.nPreamBytes = 0x3,
---
>     .symbolRate.decimMode = 0x0,
>     .rxBw = 0x27,
>     .preamConf.nPreamBytes = 0x4,
89c130
<     .formatConf.nSwBits = 0x18,
---
>     .formatConf.nSwBits = 0x18, //0x10, //nr Sync bits 16 (or up to 32 if part of preamble used for sync -> best results with 24 bits)
92c133,137
<     .formatConf.fecMode = 0x0a, // manchester coding
---
> #ifndef HW_MANCHESTER_DECODE
>     .formatConf.fecMode = 0,
> #else
>     .formatConf.fecMode = 0xA,
> #endif
95c140
<     .config.biasMode = 0x1,
---
>     .config.biasMode = 0x0,
98c143
<     .txPower = 0xA73F,
---
>     .txPower = 0xA73F,          // 0xA73F --> for TX = +14dB it is needed to set CCFG_FORCE_VDDR_HH=1 in Project Settings > GCC Compiler > Symbols !! (otherwise only 12.5dB)
102c147
<     .loDivider = 0x05,
---
>     .loDivider = 0x05
104a150
> 
105a152
> // Frequency Synthesizer Programming Command
125c172,246
<     .__dummy3 = 0x0000,
---
>     .__dummy3 = 0x0000
> };
> 
> 
> // CMD_PROP_RX
> // Proprietary Mode Receive Command
> rfc_CMD_PROP_RX_t RF_cmdPropRx =
> {
>     .commandNo = 0x3802,
>     .status = 0x0000,
>     .pNextOp = 0, // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
>     .startTime = 0x00000000,
>     .startTrigger.triggerType = 0x0,
>     .startTrigger.bEnaCmd = 0x0,
>     .startTrigger.triggerNo = 0x0,
>     .startTrigger.pastTrig = 0x0,
>     .condition.rule = 0x1,
>     .condition.nSkip = 0x0,
>     .pktConf.bFsOff = 0x0,
>     .pktConf.bRepeatOk = 0x0,
>     .pktConf.bRepeatNok = 0x0,
>     .pktConf.bUseCrc = 0,
>     .pktConf.bVarLen = 0,
>     .pktConf.bChkAddress = 0x0,
>     .pktConf.endType = 0x0,
>     .pktConf.filterOp = 0x0,
>     .rxConf.bAutoFlushIgnored = 0x0,
>     .rxConf.bAutoFlushCrcErr = 0x0,
>     .rxConf.bIncludeHdr = 0,
>     .rxConf.bIncludeCrc = 0x0,
>     .rxConf.bAppendRssi = 0x0,
>     .rxConf.bAppendTimestamp = 0x0,
>     .rxConf.bAppendStatus = 0x1,
>     .syncWord = 0x547696,         // .formatConf.nSwBits = 0x18  (8 preamble bits added for better recognition)
>     //.syncWord = 0x00007696,     // .formatConf.nSwBits = 0x10
>     .maxPktLen = 0xFF,
>     .address0 = 0xAA,
>     .address1 = 0xBB,
>     .endTrigger.triggerType = 0x1,
>     .endTrigger.bEnaCmd = 0x0,
>     .endTrigger.triggerNo = 0x0,
>     .endTrigger.pastTrig = 0x0,
>     .endTime = 0x00000000,
>     .pQueue = 0, // INSERT APPLICABLE POINTER: (dataQueue_t*)&xxx
>     .pOutput = 0 // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
> };
> 
> 
> // CMD_PROP_TX
> // Proprietary Mode Transmit Command
> rfc_CMD_PROP_TX_t RF_cmdPropTx =
> {
>     .commandNo = 0x3801,
>     .status = 0x0000,
>     .pNextOp = 0, // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
>     .startTime = 0x00000000,
>     .startTrigger.triggerType = 0x0,
>     .startTrigger.bEnaCmd = 0x0,
>     .startTrigger.triggerNo = 0x0,
>     .startTrigger.pastTrig = 0x0,
>     .condition.rule = 0x1,
>     .condition.nSkip = 0x0,
>     .pktConf.bFsOff = 0x0,  
>     .pktConf.bUseCrc = 0,
>     .pktConf.bVarLen = 0x0,
>     .pktLen = 0xff,
>     .syncWord = 0x547696,
>     .pPkt = 0 // INSERT APPLICABLE POINTER: (uint8_t*)&xxx
> };
> 
> // CMD_PROP_SET_LEN
> rfc_CMD_PROP_SET_LEN_t RF_cmdPropSetLen =
> {
>     .commandNo = CMD_PROP_SET_LEN,
>     .rxLen = 0, // SET IN THE APPLICATION
nanosonde commented 3 years ago

More or less I did it same as you did (incl. 24bit Sync!) but your idea to trigger the 0x44 as address is really brilliant! I capture the first fragment till the first CRC so I can verify the CRC and then decided to proceed further or stop receiving.

Good approach. However, what packet mode are you using? I was planning to also use unlimited length packet mode as I did with the CC1101. This requires that after receiving the length byte you quickly tell the PHY the correct packet length while already receiving it. Do you think that there is enough time if we receive the first block with CRC, check the CRC of the first block and if it is correct, then set the length of the total packet using CMD_PROP_SET_LEN? As the second block always contains at least the some basic KNX telegram stuff (CTRL, SRC, DST, L/NPCI, APCI) I think it should work. However, the RF part should run independent of the KNX stack loop. I was planning to use the async receive variant which uses a callback and a fifo buffer. https://github.com/energia/EasyLink/blob/master/src/EasyLink.cpp#L95 https://github.com/energia/EasyLink/blob/master/src/easylink/EasyLink.c#L855

Basically I wanted to strip down this EasyLink library to be just KNX RF lib which can just receive and transmit KNX RF packets.

I would like to derive the CC1310Platform class from the ArduinoPlatform class and use the Energia CC13x10 core. This way we do not have to care about UART, SPI, I2C as it is already there and compatible with the KNX stack.

Currently I use only one task but with more tasks and using semaphores with timeout the looping could be prevented - but but I fear this would need a lot of changes in the knx lib - I think looping is not a big problem with eg. 1ms sleep between loops.

I think we should stick with one thread and should hide the whole TI-RTOS stuff. If we just link this main.cc then you we could use the stack probably as usual: https://github.com/nanosonde/platform-ticc13x0/blob/master/examples/arduino-blink/src/main.cpp It makes sure to call setup() and loop() as it used in Arduino projects.

The EasyLink Library was built to be used with the Energia (=Arduino) core.

What do you think?

I will also soon publish the CC13x0 platform support on PlatformIO. I am already in contact with them. So we could nicely integrate it with PlatformIO. It is not required to use CCS then. You can debug and develop using VS Code and PlatformIO if you want. Of cource CCS should still be possible as it is a "normal" Energia project that CCS supports too.

mjm987 commented 3 years ago

However, what packet mode are you using? I was planning to also use unlimited length packet mode as I did with the CC1101

I used RF_cmdPropRx where max packet length is 255, but this could be easily change to RF_cmdPropRxAdv to unlimited length.

Basically I wanted to strip down this EasyLink library to be just KNX RF lib which can just receive and transmit KNX RF packets.

I didn't like the EasyLink Layer and preferred direct RF API.

Do you think that there is enough time if we receive the first block with CRC, check the CRC of the first block and if it is correct, then set the length of the total packet using CMD_PROP_SET_LEN?

I already do it this way but this is a good point: I don't know if it is really time critical. I will check this by adding additional delay in the callback after checking the CRC.

However, the RF part should run independent of the KNX stack loop. I was planning to use the async receive variant which uses a callback and a fifo buffer.

Yes I did it this way. I think there is no need to loop once Rx is started because all is done in background. When Rx is finshes you easily could trigger a semaphore out of the callback routine (but I didn't do yet).

Here my rf_physical_layer_cc1310.cpp and cc1310_platform.cpp

cc1310_platform_and_rf_physical_layer.zip

mjm987 commented 3 years ago

As I sayd, the only real change I on the knx stack is instantiating the knx object dynamically and not static, because tirtos is started at main() an if the knx object is instantiated static, it is started before in the C++ ctors from the startup code.

I tried starting the tirtos stack as well by the C++ ctors by placing int in a attribute ((constructor)) function but this didn't work but we will find a way (I try to debug this).

mjm987 commented 3 years ago

I would like to derive the CC1310Platform class from the ArduinoPlatform class and use the Energia CC13x10 core. This way we do not have to care about UART, SPI, I2C as it is already there and compatible with the KNX stack.

Personally I don't like the Arduino and thus Energia approach but prefferre either a interrupt approach or on complexer scenarios a rtos. With the Arduino approach (looping and no control over peripherial drivers) there is not easy to run energy optimized systems nor realtime system.

nanosonde commented 3 years ago

I used RF_cmdPropRx where max packet length is 255, but this could be easily change to RF_cmdPropRxAdv to unlimited length.

After thinking about it we should stick with your solution. The KNX spec. does NOT require a device to support maximum APDU length. There is however a mimimum that is required that I cannot rememeber currently. It is around 64 bytes. My certified KNX RF equipment has a PID_MAX_APDU_LENGTH of 55 bytes. So probably having something around 128 bytes would be enough, I would guess. Also with respect to KNX Data Secure which has greater frames due to encryption stuff.

I didn't like the EasyLink Layer and preferred direct RF API.

I have looked through your example. So it is basically the already stripped down stuff from the EasyLink lib. As I said I used this lib to get to know the TI SimpleLink Propriertary Mode RF functions.

Yes I did it this way. I think there is no need to loop once Rx is started because all is done in background. When Rx is finshes you easily could trigger a semaphore out of the callback routine (but I didn't do yet).

Mhmm. During programming of the device we have some timers for the transport layer conection-oriented connections for example. But I could live with this. While the device is in programming mode, it runs the loop and in normal runtime mode later it is just triggered by the RX reception.

Here my rf_physical_layer_cc1310.cpp and cc1310_platform.cpp

cc1310_platform_and_rf_physical_layer.zip

Thanks!

nanosonde commented 3 years ago

As I sayd, the only real change I on the knx stack is instantiating the knx object dynamically and not static, because tirtos is started at main() an if the knx object is instantiated static, it is started before in the C++ ctors from the startup code.

I tried starting the tirtos stack as well by the C++ ctors by placing int in a attribute ((constructor)) function but this didn't work but we will find a way (I try to debug this).

After reviewing your code it is clear for me that there are issue with the ctors. For example in our company we have embedded coding guidelines and a quite strict QA review process. A C++ constructor of class that does more than just initializing internal variables and so on would never pass the review process. We have to provide dedicated methods for this like init(), start() and stop() if hardware or systems calls are involved. Here we have a good reason for this. And this is also the reson why we have initChip() and stopChip() members in the stack. I would advise to move all the RF init code from the ctor to the initChip() method.

I think then you also won't have any problems anymore if the RfPhysicalLayer class is instantiated in the startup code. The initChip() method is called when the RfDataLinkLayer class is enabled by the stack. This is done if knx.start() is called. So the TI-RTOS is already running.

nanosonde commented 3 years ago

I would like to derive the CC1310Platform class from the ArduinoPlatform class and use the Energia CC13x10 core. This way we do not have to care about UART, SPI, I2C as it is already there and compatible with the KNX stack.

Personally I don't like the Arduino and thus Energia approach but prefferre either a interrupt approach or on complexer scenarios a rtos. With the Arduino approach (looping and no control over peripherial drivers) there is not easy to run energy optimized systems nor realtime system.

Yes, I know what you mean. But in this case I consider the Arduino/Energia core more like a HAL (hardware abstraction layer) which helps us. And especially to have code that is portable to a variety of MCUs.

As you have probably seen the Energia core also uses the TI-RTOS by setting this up first. Every sketch runs in its own thread. Your current implementation could work fine with the Energia core IMHO. AFAIK the TI-RTOS code most resides in the ROM, right? So it does not eat up space and support sleep/standby modes. It tries to automatically reach the lowest possible power mode depending of the system status with respect to enabled peripherals and there clocks/power.

But why not have both? ;)

nanosonde commented 3 years ago

BTW: after looking into the platforms of the KNX stack there is the same kind of issue with the LinuxPlatform class. It also creates sockets and things in the ctor which is not nice. We should have some init() method there too which gets called during knx.start(). And for some early HW init we could provide a static method earlyInit() which is callable at any time (e.g. first in main()) to do early init for UART etc.

mjm987 commented 3 years ago

After reviewing your code it is clear for me that there are issue with the ctors. For example in our company we have embedded coding guidelines and a quite strict QA review process. A C++ constructor of class that does more than just initializing internal variables and so on would never pass the review process. We have to provide dedicated methods for this like init(), start() and stop() if hardware or systems calls are involved. Here we have a good reason for this. And this is also the reson why we have initChip() and stopChip() members in the stack. I would advise to move all the RF init code from the ctor to the initChip() method.

I think then you also won't have any problems anymore if the RfPhysicalLayer class is instantiated in the startup code. The initChip() method is called when the RfDataLinkLayer class is enabled by the stack. This is done if knx.start() is called. So the TI-RTOS is already running.

The problem is not only tmy RF_PhysicalLayer but all other Objects which are instatiated out of the knx initializer. Because new will crash if used before the TI-RTOS runs. I tried to start TI-RTOS out of a attribute ((constructor)) function but this does not work: This function would indeed started as before the constructors but then the other constructor are no more started because the function which initializes the TI-RTOS does not return but starts the first thread (which is the knx app). So the constructor of the knx object is not started thereafter. We must find a way how TI-RTOS might be initialized in a attribute ((constructor)) function without starting the (knx app) thread.

For example in our company we have embedded coding guidelines and a quite strict QA review process.

Personally I would not recommend using C++ with dynamic memory allocation and freeing (by new and delete) for all bare metal controllers because of memory fragmentation but for your knx stack it seems to work because it is not done too heavy.

Do you think that there is enough time if we receive the first block with CRC, check the CRC of the first block and if it is correct, then set the length of the total packet using CMD_PROP_SET_LEN?

Yes it is, I checked it! The CRC over 10 Bytes (the first fragment which is fix) takes about 10bytes8bits10cycles = 800cy ~16us which is far less than one byte needs to tranfer at 32Kbps in Manchester code. To be sure I added then additional 10k Cycles (~200us) the callback without problem but with 100k cycles it does not work anymore.

According "when it is allowed to sleep": Currently it's not clear for the user app when it is allowed to enter deep sleep. It woud be fine if the stack informs the app (eg. by the return code of knx.loop() ) if the stack still has open timer events or needs further calling. Now I do it by calling knx.loop() for a while (eg. 100ms) after sending the sensor data. I can live with this but a event based structure would be more efficient.

nanosonde commented 3 years ago

The problem is not only tmy RF_PhysicalLayer but all other Objects which are instatiated out of the knx initializer. Because new will crash if used before the TI-RTOS runs. I tried to start TI-RTOS out of a attribute ((constructor)) function but this does not work: This function would indeed started as before the constructors but then the other constructor are no more started because the function which initializes the TI-RTOS does not return but starts the first thread (which is the knx app). So the constructor of the knx object is not started thereafter. We must find a way how TI-RTOS might be initialized in a attribute ((constructor)) function without starting the (knx app) thread.

Ok. I have to check that. I am not familiar with TI-RTOS yet. I only know similiar things from FreeRTOS. Have you checked WHY exactly it crashes?

From the SYS/BIOS(TI-RTOS Kernel) docs I read the following: 1.5.1Memory ManagementThe functions new and delete are the C++ operators for dynamic memory allocation and deallocation. For TI targets, these operators use malloc() and free(). SYS/BIOS provides reentrant versions of malloc() and free() that internally use the xdc.runtime.Memory module and (by default) the ti.sysbios.heaps.HeapMem module.

Does it maybe just crash because there is not enough heap space? I did not find any place that says that the kernel must be running in order to use memory management functions? The TI-RTOS versions of malloc/free are just reentrant versions. So I do not understand why they should not be callable before scheduler start. Or do you mean that BIOS_start() also initializes the memory management and must be called before?

For example in our company we have embedded coding guidelines and a quite strict QA review process.

Personally I would not recommend using C++ with dynamic memory allocation and freeing (by new and delete) for all bare metal controllers because of memory fragmentation but for your knx stack it seems to work because it is not done too heavy.

Yes, you are perfectly right. For such MCUs it should be avoided where possible. The stack uses it only where necessary.

Do you think that there is enough time if we receive the first block with CRC, check the CRC of the first block and if it is correct, then set the length of the total packet using CMD_PROP_SET_LEN?

Yes it is, I checked it! The CRC over 10 Bytes (the first fragment which is fix) takes about 10bytes_8bits_10cycles = 800cy ~16us which is far less than one byte needs to tranfer at 32Kbps in Manchester code. To be sure I added then additional 10k Cycles (~200us) the callback without problem but with 100k cycles it does not work anymore.

Ok, cool.

According "when it is allowed to sleep": Currently it's not clear for the user app when it is allowed to enter deep sleep. It woud be fine if the stack informs the app (eg. by the return code of knx.loop() ) if the stack still has open timer events or needs further calling. Now I do it by calling knx.loop() for a while (eg. 100ms) after sending the sensor data. I can live with this but a event based structure would be more efficient.

Yes, your are right here. We should refactor that to better handle event based scenarios.

nanosonde commented 3 years ago

Does it maybe just crash because there is not enough heap space?

Specifying the Default System HeapThe BIOS module creates a default heap for use by SYS/BIOS. When Memory_alloc() is called at runtime with a NULL heap, this system heap will be used. The default system heap created by the BIOS module is a HeapMem instance. The BIOS module provides the following configuration parameters related to the system heap:•BIOS.heapSize can be used to set the system heap size.•BIOS.heapSection can be used to place the system heap.

Example Config file:
var BIOS = xdc.useModule('ti.sysbios.BIOS');
BIOS.heapSize = 0x900;
BIOS.heapSection = "systemHeap";
nanosonde commented 3 years ago 
7.7.5Using malloc() and free()
Applications can call the malloc() and free() functions. Normally these functions are provided by the RTS library 
supplied by the code generation tools. However, when you are using SYS/BIOS, 
these functions are provided by SYS/BIOS and re-direct allocations to the default system heap (see Section 7.7.2).

To change the size of the heap used by malloc(), use the BIOS.heapSize configuration parameter.

Just an idea...

nanosonde commented 3 years ago

Just for reference:

The SYS/BIOS startup sequence is logically divided into two phases—those operations that occur prior
 to the application's "main()" function being called and those operations that are performed after
 the application's "main()" function is invoked. Control points are provided at various places in each
 of the two startup sequences for user startup functions to be inserted.The "before main()" startup
 sequence is governed completely by the XDCtools runtime package. For more information about
 the boot sequence prior to main, refer to the "XDCtools Boot Sequence and Control Points" wiki page. 
The XDCtools runtime startup sequence is as follows:
  1. Immediately after CPU reset, perform target/device-specific CPU initialization (beginning at c_int00). See the "Program Loading and Running" chapter in the Assembly Language Tools User’s Guide for your target family for details on this step and the cinit() step.
  2. Prior to cinit(), run the table of "reset functions" (the xdc.runtime.Reset module provides this hook). The functions specified in the Reset.fxns[] array are called. These reset functions are called only on platforms where a reset is performed before running a program.
  3. Run cinit() to initialize C runtime environment.
  4. Run the user-supplied "first functions" (the xdc.runtime.Startup module provides this hook).
  5. Run all the module initialization functions.
  6. Run the user-supplied "last functions" (the xdc.runtime.Startup module provides this hook).
  7. Run pinit().
  8. Run main().
mjm987 commented 3 years ago

Have you checked WHY exactly it crashes?

Yes, on the first new operator which calls malloc()

Does it maybe just crash because there is not enough heap space?

No, because if I instantiate knx dynamically after running BIOS_start() it works. The problem is that memory management is under control of TI-RTOS to support reentrancy and this needs TI-RTOS initalised before using malloc(), possibly to initalize a semaphore. I don't use and can't use the XDC Configurator tool, I guess XDC does not work with GCC but only with the TI C Compiler. At least at my installation XDC does not run and the EasyLink GCC example as well does not use or reference it. So I cant use config definitions like "var BIOS = xdc.useModule('ti.sysbios.BIOS'); BIOS.heapSize = 0x900; ..."

I didn find any information about doing such configurations without XDC but I would prefer not to use a tool like this.

mjm987 commented 3 years ago

PS: It should be possible to run the controller without tirtos (SYS/BIOS) but I would prefer to keep it because it support automatic power managemt and lets the possibility open to have multiple threads.

nanosonde commented 3 years ago

PS: It should be possible to run the controller without tirtos (SYS/BIOS) but I would prefer to keep it because it support automatic power managemt and lets the possibility open to have multiple threads.

Ok, This is exactly what Energia does. It provides some precompiled libs ehich use TI-RTOS (incl. precompiled XDC stuff) which just links to your project.

Maybe you could try it and see that they do differently on startup? This code is not from me: https://github.com/nanosonde/platform-ticc13x0/blob/master/examples/arduino-blink/src/main.cpp It is generated from this tool: https://github.com/energia/ino2cpp

An older version of the core is here: https://github.com/energia/cc13xx-core This contains the TI-RTOS config for DEBUG and RELEASE builds: https://github.com/energia/cc13xx-core/tree/master/system/kernel/tirtos/builds/CC1310_LAUNCHXL

Again, just an idea on how to find out how they did it. Arduino uses static C++ under the hood. So it seems to be possible to use it somehow.

nanosonde commented 3 years ago

BTW: Do you know these projects? https://github.com/MRazvan/cc1310_simple_radio https://github.com/MRazvan/cc1310_freertos_blink https://github.com/MRazvan/cc1310_freertos_radio

nanosonde commented 3 years ago

Have you checked WHY exactly it crashes?

Yes, on the first new operator which calls malloc()

Ok, but WHY does it crash? ;-) Is malloc() internally crashing? Or does malloc() return NULL?

mjm987 commented 3 years ago

Ok, but WHY does it crash? ;-)

I traced it now and it seems that it is not malloc() which lets it crash but accessing to periphery blocks (gpio, timer, ...) which seems not to be allowed until BIOS_start() or NoRTOS_start() is called. I tried to early call NoRTOS_start() in a constructor function and later BIOS_start() in main but this seems to be not allowed: a project possibly must be either with or without RTOS (both is handled in the ROM library I guess).

One solution cold indeed be to use FreeRTOS based on a NORTOS project. Looking into the project you referenced shows that all periperial control is done under control of user program. But when doing this I guess all comfort of automatic power management is lost. TI-RTOS is quite comfortable accroding this and FreeRTOS supports no hardware at all except sheduling and memory management. (I did a lot with FreeRTOS on other platforms...)

Ok, This is exactly what Energia does. It provides some precompiled libs ehich use TI-RTOS (incl. precompiled XDC stuff) which just links to your project. Maybe you could try it and see that they do differently on startup?

This seems to be an option but I think it makes the whole stuff more complicate and dependant from even other projects. I would like to keep it as simple as possible. I will check it perhaps somewhen but for now I would like to follow the way I began because for me it's acceptable to instantiate the knx object dynamically. My priority for now is a working solution and finishing my hardware of sensors and actors.

nanosonde commented 3 years ago

Initial support for CC1310 was added with PR https://github.com/thelsing/knx/pull/94 . For more details, see the description of PR and https://github.com/thelsing/knx/blob/master/examples/knx-cc1310/README.md