Bug in hal_device_reset()

[JonasJ]

Hi,

It seems to be a bug in hal_device_reset() when using S132.

void hal_device_reset(uint8_t gpregret_value)
{
#if defined(SOFTDEVICE_PRESENT)
    (void) sd_power_reset_reason_clr(RESET_REASON_MASK); /* avoid wrongful state-readout on reboot */
#if defined(S130) || defined(S110)
    (void) sd_power_gpregret_set(gpregret_value);
#elif defined(S132)
    (void) sd_power_gpregret_set(gpregret_value, RESET_REASON_MASK);
#endif
    (void) sd_nvic_SystemReset();
#else
    NRF_POWER->RESETREAS = RESET_REASON_MASK; /* avoid wrongful state-readout on reboot */
    NRF_POWER->GPREGRET = gpregret_value;
    NVIC_SystemReset();
#endif
    NRF_MESH_ASSERT(false);
}

On line 8 above the function sd_power_gpregret_set() is called with the wrong arguments. The first arguments should be either 0 or 1 to select between GPREGRET and GPREGRET2, the second argument should be the bits to set.

So instead of setting GPREGRET to gpregret_value, either one of the two registers or none at all is set to RESET_REASON_MASK (0xFFFFFFFF).

This bug seem to exist in v3.0.0 back to v1.0.0.

Changing line 8 to (void) sd_power_gpregret_set(0, gpregret_value); solves the problem and the correct value is written to the register.

Thanks.

[AndreasF]

Hi.

I will give you an answer tomorrow, I have to talk with someone on the Mesh team.

Best regards,

Andreas

[AndreasF]

Hi.

The developer does not want to change the implementation at this stage, but he suggest changing the documentation to the following in a future release:

For S110/S130 SoftDevice this API will set the indicated bits of the GPREGRET register. For S132 SoftDevice, this API will set the indicated bits of the GPREGRET0 register.

Thanks for the input.

Best regards,

Andreas

[JonasJ]

Hmm, not sure that I follow you on this one.

If I look at the function above it should do the following.

    NRF_POWER->GPREGRET = gpregret_value;

This is inline with the documentation in the header file (gpregret_value Value to set the retention register to).

However, when using S132 the following code is executed instead

    (void) sd_power_gpregret_set(gpregret_value, RESET_REASON_MASK);

Note that the first parameter to this function specifies which register, i.e GPREGRET or GPREGRET2 and the second specifies what bits to set. Simply the fact that RESET_REASON_MASK is used as an argument hints that something is wrong.

If I run without a softdevice GPREGRET is set to gpregret_value.

if I run with S130/S110 GPREGRET will have the bits of gpregret_value set, which is different from above, i.e assignment vs OR.

If I run with S132 three things can happen.

• GPREGRET is set to 0xFFFFFFFF (or rather 0xFF since it's 8-bit) if gpregret_value is 0,
• GPREGRET2 is set to 0xFFFFFFFF (or rather 0xFF since it's 8-bit) if gpregret_value is 1
• sd_power_gpregret_set() probably fails and returns an error if gpregret_value > 1

The header file suggest that the direct set of GPREGRET is the correct one. To get the same behavior when using a softdevice sd_power_gpregret_clr() should probably be called as well.

/**
 * Clear the reset reason register, set the retention register, and reset the
 * device. All volatile memory will be lost. This function will never return.
 *
 * @param[in] gpregret_value Value to set the retention register to.
 */

Thanks.

[AndreasF]

Hi again.

I misunderstood a bit when I talked to the developer, he had made the changes you proposed to the master branch, and it will be available in the next release.

Sorry for the confusion, have a nice Christmas.

Best regards,

Andreas

[JonasJ]

Thanks.

I guess the differences between NRF_POWER->GPREGRET = gpregret_value; and sd_power_gpregret_set(0, gpregret_value); is not something you would like to fix?