espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.59k stars 7.27k forks source link

Floating point operations in ISR (IDFGH-60) #722

Closed simon-jouet closed 4 years ago

simon-jouet commented 7 years ago

Hi,

I've been porting some code from an existing arduino codebase to the ESP32 and running into some problems while doing float operations in an ISR. (https://github.com/espressif/esp-idf/issues/562)

This issue isn't particularly recent and looking around it's been mentioned here https://www.esp32.com/viewtopic.php?f=19&t=1292&p=5993&hilit=float+isr#p5993 and here https://www.esp32.com/viewtopic.php?f=13&t=831

In my current port I just changed all the float to double to exclude the FPU, it's far from optimal and requires quite a lot of change to the codebase. In one of the forum thread @Spritetm mentions that some thought on fixing the issue was in progress, just wondering if there has been more progress since then?

In the meantime as a temporary solution is it possible to disable the FPU at compile time? it's far from optimal but it would allow the code to remain the same and once floating point operations are allowed in ISR reverting back to normal would be straightforward.

Cheers

HubbyGitter commented 7 years ago

Having been responsible for a lot of embedded software in my professional life so far, my two cent's worth in this matter is that at least the combination of "floating point operations in ISRs" and "portability" is a complete no-go, and I am unaware of any colleague who would not agree.

It may be the case that "floating point operations in ISRs on a specific target" could be discussed. But personally, I would nevertheless rather avoid them where possible, because there is usually no need for them:

Do you have a use case where you really need (and want) this?

simon-jouet commented 7 years ago

Hi @HubbyGitter,

Thanks for the feedback, I totally agree with what you are describing and previous discussion in a different issue (linked in the original message) resulted in the same outcome.

The reason for this is that I'm porting the Marlin 3d printer firmware to the ESP32 and the existing code is very much tailored to old 8 bit platforms, there is a current effort to move to some HAL for 32 bits support. In the current implementation the stepper actuation is performed in the ISR (which is neither simple nor short) and in the short term I've been trying to get the ISR working in the ESP. Like I said in the original post I got it working by changing floats to double but it's far from optimal and a slightly less dodgy solution in the short term would be nice.

In the longer term and depending on the time I have available (and obviously the interest) I'm thinking of moving away from the ISR for the stepper and rely on either the RMT or LEDc peripherals and a ring buffer continuously filled by a task, should be much more reliable and lightweight on the CPU.

What I might try to do next is to manually save the registers of the FPU (as shown inhttps://www.esp32.com/viewtopic.php?t=1292) I had a shot at it a month or so ago without much success but I might have another go.

HubbyGitter commented 7 years ago

@simon-jouet I see... Have you considered putting the ISR code into a high-priority taks (maybe even on a dedicated ESP32 core if that's really necessary) and having the ISR just trigger that task (or maybe collect some initial data including a time stamp if required, and the task could then even wait for "the right time to continue" if required)? That should allow you to stick closely to the original implementation, no?

simon-jouet commented 7 years ago

@HubbyGitter thanks for the suggestion, I might try it this way, that's close to what @Spritetm was suggesting in the previous issue, however I'm not sure it's worth putting the effort moving to a slightly less broken solution that spending a bit more effort doing it the right way. Anyway thanks very much for your input I will definitely keep in mind when I go back to work on this.

comcomservices commented 6 years ago

@HubbyGitter, I just want to say that I disagree with your point about floating point not being needed in ISR's, I agree that you "can" work around the limitation by using fixed point arthritic and then processing later but in many cases floating point operations in the handler can clean up code significantly.

We use the ESP32 in energy monitoring applications where high speed analog acquisition is used and vectors such as instantaneous power need to be calculated on a sample by sample basis. These calculations cannot be made without scaling the analog inputs to there proper units, Amps, Volts etc.. You could do weird workarounds by making one unit x times larger then the other to keep a fixed ratio and then divide later but it just makes everything so messy, especially as you get into THD analysis, realtime voltage protection etc.

Using mutexes etc to process data once the read is finished is simply impossible as the service latency is way to high..

On previous platforms we had been using, Cortex m4, m7 performing FP and DFP operatins in interrupt handlers was common practice for these types of applications..

We use the following code to work with floating point vectors in an ISR..

` uint32_t cp0_regs[18];

portENTER_CRITICAL_ISR(&afeCriticalMutex); uint32_t cp_state = xthal_get_cpenable();

        if(!cp_state) {
            xthal_set_cpenable(1);
        }

        xthal_save_cp0(cp0_regs);   // Save FPU registers

// Work with FPU

        xthal_restore_cp0(cp0_regs);

        if(cp_state) {
            // Restore FPU registers

        } else {
            // turn it back off
            xthal_set_cpenable(0);
        }
portEXIT_CRITICAL_ISR(&afeCriticalMutex);

`

filzek commented 5 years ago

@comcomservices did tou use FP on ESP32 inside hw_timers ISR without any issues? Are they precise?

comcomservices commented 5 years ago

Yeah I did, it’s stable but operations were not percise and and I got all sorts of weird arthermitic results.. usually very close, we fought with it for months and finally switched to another platform. Good luck!

On Sat, Aug 3, 2019 at 6:17 PM filzek notifications@github.com wrote:

@comcomservices https://github.com/comcomservices did tou use FP on ESP32 inside hw_timers ISR without any issues? Are they precise?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/espressif/esp-idf/issues/722?email_source=notifications&email_token=ADD3ZSK4INX5MEQZ3EOWVRLQCYU2XA5CNFSM4DQDHS6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3PYQSQ#issuecomment-517965898, or mute the thread https://github.com/notifications/unsubscribe-auth/ADD3ZSKXTTDEHJBK6RDY7TTQCYU2XANCNFSM4DQDHS6A .

-- ____

¬ Jon Mundall

FlexSCADA | Modern Control Systems & Integration http://www.flexscada.com A Subsidiary Company of ComComServices Inc.

filzek commented 5 years ago

@comcomservices it seens FPU are not quite precise as software double is, did you try to use latest DEV 4.0 to see if the problem was solved?

igrr commented 4 years ago

Implemented in 86034ad0491bcc0beedba2c0250552a6c0b04d5f as a configurable option (disabled by default).