Migrate CLI over CDC/ACM to project open_bootloader_mbr, but stuck in cli_write

[OceanS]

Hi support, 

Firstly, environment as below, 

(1) upon SDK (nRF5_SDK_15.3.0_59ac345)

(2) Nordic Open_Bootloader (example project: Open_bootloader_usb_mbr_pca10056_debug) 

(3) Nordic 52840 DK (PCA10056)

What I am doing is just try to migrate CLI over CDC/ACM to project open_bootloader_mbr, but stuck in cli_write and would not get out as below,  

Can anyone tell me why please? Thanks a lot.

FYI, I have separate the configuration of CLI_CDC/ACM away from that of DFU_CDC/ACM as below,

(1) Setting of  CLI_CDC/ACM

//==========================================================

// <h> nrf_cli_cdc_acm - CDC ACM command line interface transport

//==========================================================
// <q> NRF_CLI_CDC_ACM_ENABLED  - Enable/disable the CLI CDC ACM module.
 

#ifndef NRF_CLI_CDC_ACM_ENABLED
#define NRF_CLI_CDC_ACM_ENABLED 1
#endif

// <o> NRF_CLI_CDC_ACM_COMM_INTERFACE - COMM interface number. 
#ifndef NRF_CLI_CDC_ACM_COMM_INTERFACE
#define NRF_CLI_CDC_ACM_COMM_INTERFACE 0
#endif

// <s> NRF_CLI_CDC_ACM_COMM_EPIN - COMM IN endpoint number.
#ifndef NRF_CLI_CDC_ACM_COMM_EPIN
#define NRF_CLI_CDC_ACM_COMM_EPIN NRF_DRV_USBD_EPIN2
#endif

// <o> NRF_CLI_CDC_ACM_DATA_INTERFACE - DATA interface number. 
#ifndef NRF_CLI_CDC_ACM_DATA_INTERFACE
#define NRF_CLI_CDC_ACM_DATA_INTERFACE 1
#endif

// <s> NRF_CLI_CDC_ACM_DATA_EPIN - DATA IN endpoint number.
#ifndef NRF_CLI_CDC_ACM_DATA_EPIN
#define NRF_CLI_CDC_ACM_DATA_EPIN NRF_DRV_USBD_EPIN1
#endif

// <s> NRF_CLI_CDC_ACM_DATA_EPOUT - DATA OUT endpoint number.
#ifndef NRF_CLI_CDC_ACM_DATA_EPOUT
#define NRF_CLI_CDC_ACM_DATA_EPOUT NRF_DRV_USBD_EPOUT1
#endif

(2) Setting of DFU_CDC/ACM

#define CDC_ACM_COMM_INTERFACE          2//0
#define CDC_ACM_COMM_EPIN               NRF_DRV_USBD_EPIN4//NRF_DRV_USBD_EPIN2
#define CDC_ACM_DATA_INTERFACE          3//1
#define CDC_ACM_DATA_EPIN               NRF_DRV_USBD_EPIN3//NRF_DRV_USBD_EPIN1
#define CDC_ACM_DATA_EPOUT              NRF_DRV_USBD_EPOUT2//NRF_DRV_USBD_EPOUT1

Regards

J.S.

[Einar Thorsrud]

Hi,

Can explain/show all the change you have done in the bootloader in order to add CLI support? Out of curiosity I also wonder why you want this in the bootloader?

[OceanS]

Hi Einar, 

Thanks for your response. Let me explain you the second question firstly, 

Re-

Einar Thorsrud said:

Out of curiosity I also wonder why you want this in the bootloader?

The product would only talk to an another embedded system via CDC/ACM over USB. So, CLI would be the only way to do maintainance once it is published to customer side. 

J.S. 

[OceanS]

Hi Einar, 

Einar Thorsrud said:

Can explain/show all the change you have done in the bootloader in order to add CLI support?

Just try to migrate NRF_CLI module to the example project open_bootloader_usb_mbr_pca10056_debug, 

(1) In SDK_Config.h as below,  mainly to enable NRF_CLI related options and queue etc. Please refer to the files below for details,

4774.sdk_config.h

(2) add nrf_cli.c nrf_cli_cdc_acm.c nrf_queue.c nrf_sortlist.c

to nRF_Libraries folder in the project IDE

(3) Separate CLI_ACM/CDC settings with DFU_ACM/CDC as I already showed in my first post above. 

(4) The main NRF_CLI init/start/process as 

#include <stdio.h>
#include <stdbool.h>
#include <stddef.h>

#include "app_timer.h"
#include "nrf_cli.h"
//#include "nrf_cli_rtt.h"
#include "nrf_cli_types.h"

#include "nrf_log.h"
#include "nrf_log_ctrl.h"
#include "nrf_log_default_backends.h"
#include "nrf_log_backend_flash.h"
#include "nrf_fstorage_nvmc.h"

#if defined(APP_USBD_ENABLED) && APP_USBD_ENABLED
#define CLI_OVER_USB_CDC_ACM 1
#else
#define CLI_OVER_USB_CDC_ACM 0
#endif

#if CLI_OVER_USB_CDC_ACM
#include "nrf_cli_cdc_acm.h"
#include "nrf_drv_usbd.h"
#include "app_usbd_core.h"
#include "app_usbd.h"
#include "app_usbd_string_desc.h"
#include "app_usbd_cdc_acm.h"
#endif //CLI_OVER_USB_CDC_ACM

#if defined(TX_PIN_NUMBER) && defined(RX_PIN_NUMBER)
#define CLI_OVER_UART 1
#else
#define CLI_OVER_UART 0
#endif

/* Counter timer. */
APP_TIMER_DEF(m_timer_0);

/* Declared in demo_cli.c */
//extern uint32_t m_counter;
//extern bool m_counter_active;

/* If enabled then CYCCNT (high resolution) timestamp is used for the logger. */
#define USE_CYCCNT_TIMESTAMP_FOR_LOG 0


/**
 * @brief Command line interface instance
 * */
#define CLI_EXAMPLE_LOG_QUEUE_SIZE  (4)

#if CLI_OVER_USB_CDC_ACM
NRF_CLI_CDC_ACM_DEF(m_cli_cdc_acm_transport);
NRF_CLI_DEF(m_cli_cdc_acm,
            "mst_cli:~$ ",
            &m_cli_cdc_acm_transport.transport,
            '\r',
            CLI_EXAMPLE_LOG_QUEUE_SIZE);
#endif //CLI_OVER_USB_CDC_ACM

void cli_start(void)
{
    ret_code_t ret;

#if CLI_OVER_USB_CDC_ACM
    ret = nrf_cli_start(&m_cli_cdc_acm);
    APP_ERROR_CHECK(ret);
#endif

#if CLI_OVER_UART
    ret = nrf_cli_start(&m_cli_uart);
    APP_ERROR_CHECK(ret);
#endif

}

void cli_init(void)
{
    ret_code_t ret;

#if CLI_OVER_USB_CDC_ACM
    ret = nrf_cli_init(&m_cli_cdc_acm, NULL, true, true, NRF_LOG_SEVERITY_INFO);
    APP_ERROR_CHECK(ret);
#endif

#if CLI_OVER_UART
    nrf_drv_uart_config_t uart_config = NRF_DRV_UART_DEFAULT_CONFIG;
    uart_config.pseltxd = TX_PIN_NUMBER;
    uart_config.pselrxd = RX_PIN_NUMBER;
    uart_config.hwfc    = NRF_UART_HWFC_DISABLED;
    ret = nrf_cli_init(&m_cli_uart, &uart_config, true, true, NRF_LOG_SEVERITY_INFO);
    APP_ERROR_CHECK(ret);
#endif

    NRF_LOG_INFO("mst global Available commands:");
    NRF_LOG_INFO("- mstglobal");

}

void cli_process(void)
{
#if CLI_OVER_USB_CDC_ACM
    nrf_cli_process(&m_cli_cdc_acm);
#endif

#if CLI_OVER_UART
    nrf_cli_process(&m_cli_uart);
#endif

}

(4) I get the NRF_CLI to envoke the NRF_CLI init/start/process as,

uint32_t nrf_dfu_init_user(void)
{
    ret_code_t ret;
    NRF_LOG_DEBUG("In mst nrf_dfu_init_user");

    app_usbd_class_inst_t const * class_cli_cdc_acm = app_usbd_cdc_acm_class_inst_get(&nrf_cli_cdc_acm);
    ret = app_usbd_class_append(class_cli_cdc_acm);
    APP_ERROR_CHECK(ret);

    cli_init();
    cli_start();
    return NRF_SUCCESS;
}

ret_code_t nrf_bootloader_init(nrf_dfu_observer_t observer)
{
    NRF_LOG_DEBUG("In nrf_bootloader_init");
    ....
    ....
    if (dfu_enter)
    {
        nrf_bootloader_wdt_init();
        scheduler_init();
        dfu_enter_flags_clear();

        nrf_bootloader_dfu_inactivity_timer_restart(initial_timeout, inactivity_timeout);

        ret_val = nrf_dfu_init(dfu_observer); //here already complete usb transport initialization
        if (ret_val != NRF_SUCCESS)
        {
            return NRF_ERROR_INTERNAL;
        }

        // Call user-defined init function if implemented
        ret_val = nrf_dfu_init_user(); //here I envoke to do NRF_CLI init/start
        if (ret_val != NRF_SUCCESS)
        {
            return NRF_ERROR_INTERNAL;
        }

        NRF_LOG_DEBUG("Enter main loop");
        loop_forever(); // This function will never return.
        NRF_LOG_ERROR("Unreachable");
    }
    .....
}

static void loop_forever(void)
{
    while (true)
    {
        //feed the watchdog if enabled.
        nrf_bootloader_wdt_feed();

        app_sched_execute();
        
        cli_process();  //***************** here I get CLI_PROCESS called
        UNUSED_RETURN_VALUE(NRF_LOG_PROCESS());
        /* Sleep CPU only if there was no interrupt since last loop processing */
        __WFE();

       /*
        if (!NRF_LOG_PROCESS())
        {
            wait_for_event();
        }*/
    }
}

(5) I didn't get task_manager enabled. 

Above all is my change. Any thought please? Thanks

Regards

J.S. 

[Einar Thorsrud]

Hi,

I do not immediately see anything that sticks out.

Have you made sure that the bootloader start address is moved sufficiently down so that the bootloader does not overlap with the two last pages (MBR params and Bootloader settings). If so, the code that happens to be placed there would be corrupted. This is a problem since the SES bootloader projects in the SDK are slightly misconfigured, and you will not get a linker error even if the bootloader is up to two pages too big. And in this case, any odd error could happen.

I still don't understand why you want this in the bootloader though. I understand that you need the USB bootloader for updating firmware via USB. But why do you need CLI in addition? What will it be used for? You write that "CLI would be the only way to do maintainance once it is published to the customer side", but the bootloader should not need any "maintenance"?

[OceanS]

Hi,

Thanks for your reply. I have figured out a workaround for this, just commenting out those parts in cli_write. Seems that there is a timing issue there, like when the source code in the cycle waiting for TX_DONE to change tx_rdy, however this while loop blocks the CPU and would not let other procedure continue any more. It is very easy to reproduce such an issue if your developer would like to do this. 

However I don't know if there is any side effect. Could it get CLI consume much cpu? or something else?please let me know. Thanks.

Regards

J.S.

[Einar Thorsrud]

Hi,

It is clear that tx_rdy is not set when waiting for it, for some reason. We have found one bug where APP_USBD_CDC_ACM_USER_EVT_PORT_CLOSE is not properly handled. Is that the case here? If so, the fix could be to put the following line when handling the APP_USBD_CDC_ACM_USER_EVT_PORT_CLOSE event in cdc_acm_user_ev_handler() in nrf_cli_cdc_acm.c:

mp_internal->p_cb->handler(NRF_CLI_TRANSPORT_EVT_TX_RDY, mp_internal->p_cb->p_context);

If your workaround is to comment out the busy-wait (loop), then this in itself should not increase CPU usage. However, I suspect you might get corrupt logs or a bad state in this case since you are potentially continuing before write operations, etc. are completed.

[OceanS]

Hi,

Re-APP_USBD_CDC_ACM_USER_EVT_PORT_CLOSE, I don't think so because at this moment the port physically is still going on firmly. For CLI, it is acceptable to get some packages lost. 

Actually so far I am not so satisfied with my current software structure because everything is running in a big forever loop and I couldn't predict any CPU consumption or CPU block. I am fond of your task_manager, however I hesitate a bit due to reasons as below, 

(1) Not sure how mature your task_manager is. Is it still in experiment period?  

(2) Not enough support document to get me know how to migrate an example to task_manager. I know there is only one example BLE_APP_CLI which is developed upon task_manager. However it is still not enough. For example, how to migrate a bootloader file transfer instance to task_manager?

 Any suggestion? Thanks

J.S.

[Einar Thorsrud]

Hi,

1. Yes, the task_manager is still experimental.

2. We do not have any documentation for this (or similar). I advise against doing significant changes in the bootloader unless you have a good reason for it. We have put significant effort into the bootloader example, and it should work more or less out of the box in most use cases.

[OceanS]

Hi, 

Einar Thorsrud said:

I advise against doing significant changes in the bootloader unless you have a good reason for it. We have put significant effort into the bootloader example, and it should work more or less out of the box in most use cases.

I never wonder about the core implementation of bootloader, otherwise I would not select this as my base. However, please jump out the scope of the core feature of a bootloader. Let's start from a view of a commercial product, just imagine in some case after an application upgrade,

(1) The main application run into some panic situations, for example it would repeatedly reset the system before enter DFU mode (usb_dfu_bootloader_start_finalize).

(2) The worst case that this product would run in remote area and unattended, as well for some reason there is no button for bootloader-enter, which would be no use in most cases for an remote&unattended product.

So, could we get some assistance in bootloader to rescue the product? Seems that it could, just like uboot or some others. I don't expect the bootloader running in Nordic would be as complicate as uboot, but I need some methods to help the main application. 

rgs

J.S. 

[Einar Thorsrud]

Hi,

Can you elaborate on what you would like the bootloader to do, when and how? If the idea is to have a way to recover from faulty application using something like a golden image, then you could think of this as an alternative that requires minimal changes to the bootloader (I don't know how it fits with your product, though):

• If the bootloader detects that the reset reason is watchdog reset or CPU lock-up, enter DFU mode.
• When nRF enters DFU mode unexpectedly, the host CPU can assume that there is a problem with the application, and do some of the following (whatever is appropriate in your case):
  • choose to perform a DFU update (if that is what you need to recover)
  • delete flash regions used to store application data (if that is what causes problems). This could be triggered by the bootloader itself if safe, or you could expand the DFU protocol, etc. This is perhaps a task where you would use CLI?

However, depending on your application, it would also make sense to investigate to what degree the application is capable of "messing itself up". Generally, adding more logic (which can potentially introduce more bugs an issues) to fix issues that may never occur is not necessarily a good idea.

[OceanS]

Hi Einar, 

Thanks for your response. Partially agree with you 

Einar Thorsrud said:

adding more logic (which can potentially introduce more bugs an issues)

Anyway, while I am still not satisfied with my current implementation as I stated above like no mature task management etc., just let's close this ticket since I already got a workaround.

Thanks again for your time.  

Regards

J.S.