Problems with structs and pointers (does not save value). Works on ESP32

[DJPX]

I am not sure if it caused by the megaTinyCore but it seems possible since I did not found much other possibilitys.
Serveral other micro controllers with different cores do not have problems with this code and save the data of the struct.

When I run this code:
`#include <Arduino.h>

struct SimpleStruct
{
SimpleStruct()
{
}
SimpleStruct *nextSimpleStruct = nullptr;

};

SimpleStruct *rootSimpleStruct;

int led = 4;
// the setup function runs once when you press reset or power the board
void setup()
{
// initialize digital pin as an output.
pinMode(led, OUTPUT);
rootSimpleStruct = new SimpleStruct();
}

// the loop function runs over and over again forever
void loop()
{
if (rootSimpleStruct != nullptr)
{
digitalWrite(led, HIGH);
}
else
{
digitalWrite(led, HIGH);
delay(1000);
digitalWrite(led, LOW);
}

delay(1000); // wait for a second
}`

on my ESP32S2 it just works (LED is on) but on my ATtiny202 I still got a nullpointer (the LED blinks). It seems the nano works too.

I clearly set with rootSimpleStruct = new SimpleStruct(); a new none nullpointer element, but it seems that it does not store the address of the created element or it does not reserve memory for the element.

Are there others with that problem too?
I am using PlatformIO as IDE
with an Nano as Programmer via UPID.
(I already tried using the Arduion IDE with different versions too)

Are there resource problems or something like that?

[SpenceKonde]

I'm sorry, I can't make hide nor hair of that code, what the fuck is it supposed to do?

I may not have the CS background to understand what expected behavior is without primpting

[DJPX]

@SpenceKonde Thank you for your answer, I will try to explain it in the following text to you. Please ask if you have questions about something. Maybe I can help you somehow?

The code represents the minimal required code were the problem ocurres. (It is striped down to the problem so that you can see the problem. The original intention is not visible anymore).

The expected behavior of the code is:
LED at pin 4 is on -> everything works, pointer is not a nullpointer
LED at pin 4 is blinking -> pointer is still a nullpointer even after it got an value assigned. It means, that the rootSimpleStruct is still a nullpointer (no value was saved). My guess is that either it did not reserve the memory or it may reserved the memory but it did not return the pointer to that area so it is still an nullpointer. I tested the minimal code and the whole code on other platforms and there it did work.

Since the example is so small there should be no problem with the limited flash or ram of the ATtiny202. Maybe I am wrong about that, but the PlatformIO and the ArduionIDE bouth show that there is enought flash and ram for the code.

The complete code is used for saving alerts, for a timer. I can later add the whole code when I am back home.

How should I add the code in text or just attach it as file?

[DJPX]

I uploaded the more complete classes here: https://workupload.com/file/mvVA7yMVwjS as zip file

*Note: Disable the DEBUGGING define to save flash memory on controllers which have limited resources
*Note 2: The main class is used as testclass, this will get changed later.

#include <Arduino.h>
#include "SimpleTimer.h"
#define DEBUGGING

SimpleTimer timer;

// Pin to switch the device on and off
int8_t outputPin = 2;
int8_t currenState = LOW;

int8_t getNextState()
{
    currenState = currenState == HIGH ? LOW : HIGH;
    return currenState;
}

void testAddAlerts()
{
    // timer.addAlert(timer.convertTimeToMilliSec(0, 0, 5), B01111111);
    timer.addAlert(00, 00, 02, B01111111);
    timer.setAllAlertsActive(true);
}

void setup()
{

#ifdef DEBUGGING
    Serial.begin(115200);
#endif

    pinMode(outputPin, OUTPUT);
    testAddAlerts();
    timer.setTimer(0, 0, 0);
    digitalWrite(outputPin, getNextState());
    unsigned long future = millis() + 2000;
    while (future > millis())
    {
        delay(100);
    }
    digitalWrite(outputPin, getNextState());
}

void loop()
{

    if (timer.update() == 1)
    {
#ifdef DEBUGGING
        Serial.println("Alert");
#endif

        digitalWrite(outputPin, getNextState());
        // Resets the timer so there is an output every alert
        timer.setTimer(0, 0, 0);
        timer.setAllAlertsActive(true);
    }
}

#include <Arduino.h>
#pragma once
#define CHECKONLYEVERYSECOND
// #define DEBUGGING

struct Alert
{
    Alert(uint32_t time, uint8_t executionDays)
    {
        executionTime = time;
        setExecutionDays(executionDays);
        activate(true);
    }
    /**
     * @brief Set the execution time
     *
     * @param time
     */
    void setExecutionTime(uint32_t time)
    {
        executionTime = time;
    }

    uint8_t getExecutionDays()
    {
        return data >= 128 ? data - 128 : data;
    }

    void setExecutionDays(uint8_t bitmask)
    {
        boolean active = isActive();
        data = bitmask;
        activate(active);
    }
    /**
     * @brief Activates or deactivates the alert
     *
     * @param activate
     */
    void activate(bool activate)
    {
        if (isActive())
        {
            if (!activate)
            {
                data -= 128;
            }
        }
        else
        {
            if (activate)
            {
                data += 128;
            }
        }
    }
    /**
     * @brief Returns if the alert is currently active or not
     *
     * @return true The alert is active
     * @return false The alert is deactivated
     */
    bool isActive()
    {
        if (data >= 128)
        {
            return true;
        }
        else
        {
            return false;
        }
    }
    Alert *nextAlert = nullptr;
    /**
     * @brief data saves multiple differnt data in an 8 bit integer
     * The first bit indcates if the alert is active
     *
     *
     */
    uint8_t data;
    uint32_t executionTime = 0;
};

/**
 * @brief Keeps track of time and triggers when an alert occures
 * (Use the update function for that)
 *
 */
class SimpleTimer
{

public:
    /**
     * @brief Saves all alerts
     *
     */
    Alert *rootAlert = nullptr;
    SimpleTimer();
    ~SimpleTimer();
    /**
     * @brief Updates the time with the elapsed time and checks if an alert is triggered. Also it resets the time and the alerts when "a day" is passed.
     *
     * @return returns 1 if an alert was triggered and 0 if noting happend
     */
    int update();

    /**
     * @brief Converts the time given as normal time like 18:30:30 in milliseconds
     *
     * @param hours A value between 0-23
     * @param minutes A value between 0-59
     * @param seconds A value between 0-59
     * @return The time in milliseconds
     */
    uint32_t convertTimeToMilliSec(uint8_t hours, uint8_t minutes, uint8_t seconds);

    /**
     * @brief Adds an alert to the timer
     *
     * @param time The time when the alert should trigger
     * @param weekdays bitmask were bit 1-7 represents monday-sunday, bit 0 is not used
     */
    void addAlert(uint32_t time, uint8_t weekdays);

    /**
     * @brief Adds an alert to the timer
     *
     * @param hours A value between 0-23
     * @param minutes A value between 0-59
     * @param seconds A value between 0-59
     * @param weekdays bitmask were bit 1-7 represents monday-sunday, bit 0 is not used
     */
    void addAlert(uint8_t hours, uint8_t minutes, uint8_t seconds, uint8_t weekdays);

    /**
     * @brief Remove the alert on the given index
     *
     * @param index the position of the alert
     * @return boolean which indicates if the removing was succesfull
     */
    boolean removeAlert(int index);

    /**
     * @brief Remove the alert with the time
     *
     * @param time the time which should get removed
     * @return boolean which indicates if the removing was succesfull
     */
    boolean removeAlert(uint32_t time);

    /**
     * @brief Get the alert with the given index
     *
     * @param index An positive integer which is the desired index of the alert
     * @return Returns an Alert if it exists. If there is no Alert on this index it returns nullptr.
     */
    Alert *getAlert(uint8_t index);

    /**
     * @brief Sets all alerts inactive or active ´
     *
     * @param active Value which defines if all alerts should be active or inactive
     */
    void setAllAlertsActive(bool active);

    /**
     * @brief Sets the current time
     *
     * @param hours A value between 0-23
     * @param minutes A value between 0-59
     * @param seconds A value between 0-59
     */
    void setTimer(uint8_t hours, uint8_t minutes, uint8_t seconds);

    // Used for debugging
    void printTime(uint32_t time)
    {
#ifdef DEBUGGING
        // Uses too mutch resources so it will only used to debug
        int hours = time / (1000 * 60 * 60);
        time = time - hours * (1000 * 60 * 60);
        int minutes = time / (1000 * 60);
        time = time - minutes * (1000 * 60);
        int seconds = time / 1000;
        time = time - seconds * 1000;
        Serial.print(hours);
        Serial.print(":");
        Serial.print(minutes);
        Serial.print(":");
        Serial.print(seconds);
        Serial.print(":");
        Serial.println(time);
#endif
    }

protected:
    uint32_t lastTimeUpdate = 0;
    /**
     * @brief Represents the current daytime in milliseconds if it synced before, else it is just a virtual day with 24h
     *
     */
    uint32_t time = 0;

    // Saves the current time just here so the microncontroller does not need to reserve always new storage
    uint32_t currentTime;

    /**
     * @brief Saves the current weekday as an bit.
     * X        X           X           X           X           X           X           X
     * Notused  Monday      Tuesday     Wednesday   Thursday    Friday      Saturday    Sonntag
     */
    uint8_t currentWeekday = B01000000;

#ifdef CHECKONLYEVERYSECOND
    /**
     * @brief Stores the milliseconds elapesed since the last full second was reached
     * Only use when CHECKONLYEVERYSECOND is defined
     *
     */
    uint16_t lastSecUpdate;
#endif
};

#include "SimpleTimer.h"

SimpleTimer::SimpleTimer()
{
    time = 0;
}

SimpleTimer::~SimpleTimer()
{
}

uint32_t SimpleTimer::convertTimeToMilliSec(uint8_t hours, uint8_t minutes, uint8_t seconds)
{
    return ((((hours * 60) + minutes) * 60UL) + seconds) * 1000UL;
}

void SimpleTimer::setTimer(uint8_t hours, uint8_t minutes, uint8_t seconds)
{
    // TODO check if params are valid not negativ or to high
    // Calculates the time in milli seconds
    time = convertTimeToMilliSec(hours, minutes, seconds);
}

void SimpleTimer::setAllAlertsActive(bool activ)
{
    Alert *currentAlert = rootAlert;
    while (currentAlert != nullptr)
    {
        currentAlert->activate(activ);
        currentAlert = currentAlert->nextAlert;
    }
}

void SimpleTimer::addAlert(uint8_t hours, uint8_t minutes, uint8_t seconds, uint8_t weekdays)
{
    addAlert(convertTimeToMilliSec(hours, minutes, seconds), weekdays);
}

void SimpleTimer::addAlert(uint32_t time, uint8_t weekdays)
{
    if (rootAlert != nullptr)
    {
        Alert *currentAlert = rootAlert;
        while (currentAlert->nextAlert != nullptr)
        {
            currentAlert = currentAlert->nextAlert;
        }

        currentAlert->nextAlert = new Alert(time, weekdays);
        currentAlert->nextAlert->activate(true);
    }
    else
    {
        rootAlert = new Alert(time, weekdays);
    }
}

int SimpleTimer::update()
{
    uint8_t returnValue = 0;
    currentTime = millis();
    time += currentTime - lastTimeUpdate;

#ifdef CHECKONLYEVERYSECOND
    lastSecUpdate += currentTime - lastTimeUpdate;
    if (lastSecUpdate >= 1000)
    {
        printTime(time);
        lastSecUpdate = lastSecUpdate - 1000;
#endif
        if (rootAlert != nullptr)
        {
            Alert *currentAlert = rootAlert;
            while (currentAlert != nullptr)
            {
                if (currentAlert->isActive() && currentAlert->executionTime <= time && (currentAlert->getExecutionDays() & currentWeekday) > 0)
                {
                    currentAlert->activate(false);
                    returnValue = 1;
                }
                else
                {
                    currentAlert = currentAlert->nextAlert;
                }
            }
        }
        else
        {
            return 1;
        }

        // Test if "a day" has passed
        if (time >= 86400000L)
        {
            time = time - 86400000L;
            // Setting the new weekday
            currentWeekday = currentWeekday >> 1;
            // Overflow bit is set, start a new week
            if (currentWeekday == 0)
            {
                currentWeekday = B01000000;
            }

            setAllAlertsActive(true);
        }
#ifdef CHECKONLYEVERYSECOND
    }
#endif
    lastTimeUpdate = currentTime;
    return returnValue;
}

Alert *SimpleTimer::getAlert(uint8_t index)
{
    Alert *askedAlert = rootAlert;
    for (int8_t i = 0; i < index; i++)
    {

        if (index == i)
        {
            return askedAlert;
        }
        if (askedAlert->nextAlert != nullptr)
        {
            askedAlert = askedAlert->nextAlert;
        }
        else
        {
            return nullptr;
        }
    }
    return askedAlert;
}

[SpenceKonde]

I'm not sure I'm familiar enough with these aspects of the language to understand what is happening that is not expected, or help diagnose it, but this seems to be well outside of the realm of stuff that we expect to depend on the architecture or core. That's the C++ language that seems to be at hand here, nothing core or avr specific? You're gonna need to wait for someone else to come by who can better interpret your question and either point me to something I can understand, or tell you where you're going wrong.

[DJPX]

The code is actually working fine, as long it is on the ESP-Arduinocore or the MegaCoreX (1.1.0). But it seems to fail when I use the MegaTinyCore for some reason. And the only thing I found out while debugging, was that when I use a pointer to a struct, I was not able to save a value
(e.g. rootSimpleStruct = new SimpleStruct(); did result into rootSimpleStruct = nullpointer;)
Is this commit already in the latest version which you can use on the arduion IDE? ( Do not think so but I am not sure).

I do not know mutch about the meagTinyCore so far, but if I can help somehow just ask and I give it my best.

[MX682X]

My best guess, based on looking at the code, is that you are running out of RAM when calling new/malloc. When malloc is returning a nullpointer, it means the memory allocation has failed. Neither code is checking, if the allocation failed. And the 202 has only 128 Bytes of SRAM.

[MX682X]

@SpenceKonde maybe it would be a good idea to change the __malloc_margin variable somewhere in the init code for 128B and 256B RAM devices to 32 and 64 respectively? To avoid having a broken malloc....

[SpenceKonde]

I mean, malloc in the context of embedded systems is a construct of pure evil provided by Satan himself to harm humanity....

But it's part of stdlib; I reckon we ought to ensure that malloc is usable on all parts.

(even though using it on low ram parts sets one down poison-ivy lined path through some brambles and to deadly nightshade growing on an arbor with a placard reading "Delicious wild grapes".... on top of a nest of fire ants, and just like the aforementioned path in a forest, I would suggest not turning down that path at all.

But if the __malloc_margin is used to kneecap malloc, that's a design decision that ought to be explicitly stated "This is set to this value. This makes malloc unusable on parts with less than X ram, and that is intended because they have so little ram that malloc is impractical on those devices" And it sounds like that was not documented, and hence I'm not confident that that's a wise design decision or a boneheaded one.

First glance, 128 looks like a luxuious and inappropriate indulgance, especially on parts without enough ram. I agree that , barring discovery of a statement explaining why 128 was chosen without consideration of the memory on the part, that value should be lower on parts with low memory. 64 I have no qualms about. 32 starts to make me nervous, Consider an attached interrupt (also a terrible idea on those parts because of the limited flash). That will end up pushing 1 2-byte return address, the 14 call-clobbered registers, plus the 0 and 1 register, plus the sreg, plus IIRC it needs 4 registers to store state during the attached pin interrupt handler, despite my valiant attempts to make it suck less, so that's right there 33 bytes, then 2 more for the return address from the icall, that's 35 that any attached pin interrupt will, guaranteed, push. And reckless use of memory could push that to 51 in a worst case - PLUS any local variables that don't fit in registers and are created on the stack. So we can't go all the way down to 32 as you suggest. But 128 still seems a bit excessive, and I think 64 is okay because if you're storing variables on the stack on an ISR on a part with 256 bytes of ram, you're planting the seeds of poor code and reaping the bitter fruit of stack-heap collisions and should stop doing that.

[SpenceKonde]

@MX682X Well, I think I just showed why they put that patch in - because it was 32, and with 32, you are practically there just by calling a function from within an ISR!

[MX682X]

Huh, never thought that much about it. Thanks for the breakdown

[DJPX]

Sorry for the late response I had todo alot of other things. The __malloc_margin actually helped. But is still had to redesign the code since I run out of flash memory ( I am pretty mutch working on the limits here).
Thank you, for your post where you explain allitle bit more about the problem.

[SpenceKonde]

@MX682X - Out of curiosity, where the fuck is __malloc_margin actually defined? I cannot find anyplace that actually defines it, so I don't know how I could change it. I'm not sure it is a good idea to change, though, nor am I sure it even has to, because I think malloc() burns enough flash that it's not usable on 2k parts, and maybe I should just note that malloc is not an option with the 2k parts.

But I'd like to be able to change it if I ever had to, yaknow? There are a lot of very important files that are so thoroughly buried that it's impossible to locate them I searched the toolchain, the core, the source for avr-gcc, and the source for avrlibc. I found lots of places where it was mentioned, and places where it was declared as an extern, but no actual definition of it. So I don't have a goddamned clue how it becomes set.

[MX682X]

https://github.com/avrdudes/avr-libc/blob/main/libc/stdlib/malloc.c

My Idea was to simply add that to init or so. At least it would make malloc work
#if RAMSIZE <=256
__malloc_margin = 64;
#endif