Skip to content

Commit

Permalink
avr: simplify stepper ISR. still test failures
Browse files Browse the repository at this point in the history
  • Loading branch information
@gin66
gin66 committed Apr 28, 2024
1 parent c96166d commit ea44b43
Show file tree
Hide file tree
Showing 7 changed files with 200 additions and 68 deletions.
5 changes: 4 additions & 1 deletion examples/Issue250/Issue250.ino
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ uint16_t loopcnt = 0;
#ifndef USE_MOVETO
#define USE_MOVETO true
#endif
#ifndef LOOPS
#define LOOPS 200
#endif

uint32_t previous_runtime = 0;

Expand Down Expand Up @@ -79,7 +82,7 @@ void loop() {
}
previous_runtime = runtime;

if (loopcnt == 200) {
if (loopcnt == LOOPS) {
stepper->stopMove();
while (stepper->isRunning()) {
}
Expand Down
1 change: 1 addition & 0 deletions extras/tests/simavr_based/test_issue250_30us/.gitignore
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
src/
131 changes: 131 additions & 0 deletions extras/tests/simavr_based/test_issue250_30us/Makefile
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
#
# In order to execute the test for one directory use:
#
# make -C test_sd_01b_328p -f ../Makefile.test

SRC=$(wildcard ../../../src/*) $(wildcard src/*)

# platformio should contain only one env section.
# This section states the dut name
# atmega168
# atmega168p
# atmega328
# atmega328p
# atmega2560_timer1
# atmega2560_timer3
# atmega2560_timer4
# atmega2560_timer5
#

DUT=$(shell gawk '/env:/{print(substr($$1,6,length($$1)-6))}' platformio.ini)

TRACES=-at StepISR=trace@0x25/0x08 # PB3
TRACES+=-at FillISR=trace@0x25/0x10 # PB4

#
ifeq ($(DUT),atmega2560_timer1)
DEVICE=atmega2560
TRACES+=-at StepA=trace@0x025/0x20 #OC1A PB5 11 ATMega2560
TRACES+=-at StepB=trace@0x025/0x40 #OC1B PB6 12 ATMega2560
TRACES+=-at StepC=trace@0x025/0x80 #OC1C PB7 13 ATMega2560
#
else ifeq ($(DUT),atmega2560_timer3)
DEVICE=atmega2560
TRACES+=-at StepA=trace@0x02e/0x08 #OC3A PE3 5 ATMega2560
TRACES+=-at StepB=trace@0x02e/0x10 #OC3B PE4 2 ATMega2560
TRACES+=-at StepC=trace@0x02e/0x20 #OC3C PE5 3 ATMega2560
#
else ifeq ($(DUT),atmega2560_timer4)
DEVICE=atmega2560
TRACES+=-at StepA=trace@0x102/0x08 #OC4A PH3 6 ATMega2560
TRACES+=-at StepB=trace@0x102/0x10 #OC4B PH4 7 ATMega2560
TRACES+=-at StepC=trace@0x102/0x20 #OC4C PH5 8 ATMega2560
#
else ifeq ($(DUT),atmega2560_timer5)
DEVICE=atmega2560
TRACES+=-at StepA=trace@0x10b/0x08 #OC5A PL3 46 ATMega2560
TRACES+=-at StepB=trace@0x10b/0x10 #OC5B PL4 45 ATMega2560
TRACES+=-at StepC=trace@0x10b/0x20 #OC5C PL5 44 ATMega2560

else ifeq ($(DUT),atmega168)
DEVICE=atmega168
TRACES+=-at StepA=trace@0x25/0x02 #OC1A PB1 9 atmega168
TRACES+=-at StepB=trace@0x25/0x04 #OC1B PB2 10 atmega168

else ifeq ($(DUT),atmega168p)
DEVICE=atmega168p
TRACES+=-at StepA=trace@0x25/0x02 #OC1A PB1 9 atmega168p
TRACES+=-at StepB=trace@0x25/0x04 #OC1B PB2 10 atmega168p

else ifeq ($(DUT),atmega328)
DEVICE=atmega328
TRACES+=-at StepA=trace@0x25/0x02 #OC1A PB1 9 ATMega328
TRACES+=-at StepB=trace@0x25/0x04 #OC1B PB2 10 ATMega328

else ifeq ($(DUT),atmega328p)
DEVICE=atmega328p
TRACES+=-at StepA=trace@0x25/0x02 #OC1A PB1 9 ATMega328p
TRACES+=-at StepB=trace@0x25/0x04 #OC1B PB2 10 ATMega328p

else ifeq ($(DUT),atmega32u4)
DEVICE=atmega32u4
TRACES+=-at StepA=trace@0x025/0x20 #OC1A PB5 11
TRACES+=-at StepB=trace@0x025/0x40 #OC1B PB6 12
#TRACES+=-at StepC=trace@0x025/0x80 #OC1C PB7 13

endif

ifeq ($(DEVICE),atmega2560)
TRACES+=-at DirA=trace@0x2b/0x01 # Pin 21 PD0
TRACES+=-at DirB=trace@0x2b/0x02 # Pin 20 PD1
TRACES+=-at DirC=trace@0x10b/0x80 # Pin 42 PL7
TRACES+=-at EnableA=trace@0x2b/0x04 # Pin 19 PD2
TRACES+=-at EnableB=trace@0x2b/0x08 # Pin 18 PD3
TRACES+=-at EnableC=trace@0x10b/0x40 # Pin 43 PL6

else ifeq ($(DEVICE),$(filter $(DEVICE),atmega168 atmega168p atmega328 atmega328p))
TRACES+=-at DirA=trace@0x2b/0x20 # Pin 5 PD5
TRACES+=-at DirB=trace@0x2b/0x80 # Pin 7 PD7
TRACES+=-at EnableA=trace@0x2b/0x40 # Pin 6 PD6
TRACES+=-at EnableB=trace@0x25/0x01 # Pin 8 PB0

else ifeq ($(DUT),atmega32u4)
TRACES+=-at DirA=trace@0x25/0x10 # Pin 26 PB4
TRACES+=-at DirB=trace@0x25/0x08 # Pin 14 PB3
#TRACES+=-at DirC=trace@0x10b/0x80 # Pin 42 PL7
TRACES+=-at EnableA=trace@0x25/0x04 # Pin 16 PB2
TRACES+=-at EnableB=trace@0x25/0x02 # Pin 15 PB1
#TRACES+=-at EnableC=trace@0x10b/0x40 # Pin 43 PL6

endif

FIRMWARE=".pio/build/$(DUT)/firmware.elf"

DIR=$(shell env pwd)

ifndef SILENCE
SILENCE=0
endif

test: .tested

.tested: result.txt ../judge_pos0.awk
echo DUT=$(DUT)
rm -f .tested
gawk -f ../judge_pos0.awk -v DIR=$(DIR) result.txt
test -f .tested

result.txt: x.vcd
gawk -v SILENCE=$(SILENCE) -f ../eval.awk x.vcd >/dev/null

x.vcd: $(SRC) ../run_avr platformio.ini src/Issue250.ino
~/.platformio/penv/bin/pio run -e $(DUT) || ~/.local/bin/pio run -e $(DUT) || env pio run -e $(DUT)
../run_avr $(FIRMWARE) -m $(DEVICE) -o x.vcd $(TRACES)

src/Issue250.ino:
mkdir -p src
cd src; ln -s ../../../../../examples/Issue250/Issue250.ino .

clean:
rm -fR .pio .tested x.vcd result.txt

Empty file added extras/tests/simavr_based/test_issue250_30us/expect.txt
View file
Empty file.
32 changes: 32 additions & 0 deletions extras/tests/simavr_based/test_issue250_30us/platformio.ini
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
; PlatformIO Project Configuration File
;
; Build options: build flags, source filter
; Upload options: custom upload port, speed and extra flags
; Library options: dependencies, extra library storages
; Advanced options: extra scripting
;
; Please visit documentation for the other options and examples
; https://docs.platformio.org/page/projectconf.html

[platformio]

# There should be only one env section for the DUT under test.
# One of
# atmega168p
# atmega328p
# atmega2560_timer1
# atmega2560_timer3
# atmega2560_timer4
# atmega2560_timer5
#
[common]
# This is the line input to StepperDemo:
#build_flags = -D SIMULATOR -D SIMAVR_TIME_MEASUREMENT -D BLOCK_INTERRUPT_US=30
build_flags = -D SIMULATOR -D SIMAVR_TIME_MEASUREMENT -D BLOCK_INTERRUPT_US=30 -D LOOPS=100 -D TOGGLE_DIRECTION=false -D USE_MOVETO=false

[env:atmega328p]
platform = atmelavr
board = nanoatmega328
framework = arduino
build_flags = -Werror -Wall ${common.build_flags}
lib_extra_dirs = ../../../../..
2 changes: 1 addition & 1 deletion src/StepperISR.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class StepperQueue {
volatile bool _prepareForStop;
volatile bool _isRunning;
inline bool isRunning() { return _isRunning; }
inline bool isReadyForCommands() { return true; }
inline bool isReadyForCommands() { return !_prepareForStop; }
enum channels channel;
#endif
#if defined(SUPPORT_SAM)
Expand Down
97 changes: 31 additions & 66 deletions src/StepperISR_avr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,10 @@
// Toggle is 0 1
// Disconnect is 0 0
//
#define Stepper_OneToZero(T, X) TCCR##T##A = TCCR##T##A & ~_BV(COM##T##X##0)
#define Stepper_Zero(T, X) \
TCCR##T##A = (TCCR##T##A | _BV(COM##T##X##1)) & ~_BV(COM##T##X##0)
#define Stepper_Toggle(T, X) \
TCCR##T##A = (TCCR##T##A | _BV(COM##T##X##0)) & ~_BV(COM##T##X##1)
#define Stepper_One(T, X) TCCR##T##A |= _BV(COM##T##X##1) | _BV(COM##T##X##0)
#define Stepper_Disconnect(T, X) \
TCCR##T##A &= ~(_BV(COM##T##X##1) | _BV(COM##T##X##0))
#define Stepper_IsOne(T, X) \
Expand Down Expand Up @@ -71,6 +69,7 @@
#define EnableCompareInterrupt(T, X) TIMSK##T |= _BV(OCIE##T##X)
#define ClearInterruptFlag(T, X) TIFR##T = _BV(OCF##T##X)
#define SetTimerCompareRelative(T, X, D) OCR##T##X = TCNT##T + D
#define InterruptFlagIsSet(T, X) ((TIFR##T & _BV(OCF##T##X)) != 0)

#define ConfigureTimer(T) \
{ \
Expand Down Expand Up @@ -149,60 +148,36 @@ void StepperQueue::init(uint8_t queue_num, uint8_t step_pin) {
return; \
} \
struct queue_entry* e = &fas_queue_##CHANNEL.entry[rp & QUEUE_LEN_MASK]; \
/* There is a risk, that this new compare time is delayed by one cycle */ \
OCR##T##CHANNEL += e->ticks; \
if (Stepper_IsOneIfOutput(T, CHANNEL)) { \
/* Clear output bit by another compare event */ \
Stepper_OneToZero(T, CHANNEL); \
ForceCompare(T, CHANNEL); \
if (e->steps-- > 1) { \
/* perform another step with this queue entry */ \
Stepper_One(T, CHANNEL); \
exitStepperISR(); \
return; \
} \
} else if (fas_queue_##CHANNEL._prepareForStop) { \
/* new command received after running out of commands */ \
/* if this new command requires a step, then this step would be lost \
*/ \
fas_queue_##CHANNEL._prepareForStop = false; \
if (e->toggle_dir) { \
Stepper_ToggleDirection(CHANNEL); \
/* There is a risk, that the new compare time is delayed by one cycle */ \
uint16_t ticks = e->ticks; \
/* Clear output bit by another compare event */ \
ForceCompare(T, CHANNEL); \
if (e->steps == 0) { \
/* mo more steps with this queue entry */ \
if (TEST_NOT_REPEATING_ENTRY) { \
rp++; \
fas_queue_##CHANNEL.read_idx = rp; \
e = &fas_queue_##CHANNEL.entry[rp & QUEUE_LEN_MASK]; \
} \
if (e->steps > 0) { \
/* That's the problem, so generate a step */ \
Stepper_One(T, CHANNEL); \
ForceCompare(T, CHANNEL); \
/* Use a high time of 3us */ \
delayMicroseconds(3); \
/* Clear output bit by another toggle */ \
Stepper_OneToZero(T, CHANNEL); \
ForceCompare(T, CHANNEL); \
if (e->steps-- > 1) { \
/* perform another step with this queue entry */ \
Stepper_One(T, CHANNEL); \
exitStepperISR(); \
return; \
Stepper_Zero(T, CHANNEL); \
if (rp != fas_queue_##CHANNEL.next_write_idx) { \
/* command in queue */ \
if (e->toggle_dir) { \
Stepper_ToggleDirection(CHANNEL); \
} \
if (e->steps != 0) { \
Stepper_Toggle(T, CHANNEL); \
e->steps--; \
} \
} else { \
/* queue empty: wait this command to complete, then disconnect */ \
fas_queue_##CHANNEL._prepareForStop = true; \
} \
} \
if (TEST_NOT_REPEATING_ENTRY) { \
rp++; \
} \
fas_queue_##CHANNEL.read_idx = rp; \
if (rp != fas_queue_##CHANNEL.next_write_idx) { \
/* command in queue */ \
e = &fas_queue_##CHANNEL.entry[rp & QUEUE_LEN_MASK]; \
if (e->toggle_dir) { \
Stepper_ToggleDirection(CHANNEL); \
} \
if (e->steps != 0) { \
Stepper_One(T, CHANNEL); \
} \
} else { \
/* queue is empty, so wait this command to complete, then disconnect */ \
fas_queue_##CHANNEL._prepareForStop = true; \
else { \
e->steps--; \
} \
SetTimerCompareRelative(T, CHANNEL, ticks); \
exitStepperISR(); \
}

Expand Down Expand Up @@ -246,10 +221,10 @@ AVR_CYCLIC_ISR_GEN(FAS_TIMER_MODULE)
/* ensure no compare event */ \
SetTimerCompareRelative(T, CHANNEL, 32768); \
/* set output one, if steps to be generated */ \
if (e->steps > 0) { \
Stepper_One(T, CHANNEL); \
} else { \
Stepper_Zero(T, CHANNEL); \
Stepper_Zero(T, CHANNEL); \
if (e->steps != 0) { \
Stepper_Toggle(T, CHANNEL); \
e->steps--; \
} \
/* clear interrupt flag */ \
ClearInterruptFlag(T, CHANNEL); \
Expand All @@ -258,34 +233,24 @@ AVR_CYCLIC_ISR_GEN(FAS_TIMER_MODULE)
/* start */ \
SetTimerCompareRelative(T, CHANNEL, 10);

#define DISABLE(T, CHANNEL) DisableCompareInterrupt(T, CHANNEL)

void StepperQueue::startQueue() {
uint8_t rp;
struct queue_entry* e;

_isRunning = true;
switch (channel) {
case channelA:
DISABLE(FAS_TIMER_MODULE, A);
_isRunning = true;
_prepareForStop = false;
GET_ENTRY_PTR(FAS_TIMER_MODULE, A)
PREPARE_DIRECTION_PIN(A)
AVR_START_QUEUE(FAS_TIMER_MODULE, A)
break;
case channelB:
DISABLE(FAS_TIMER_MODULE, B);
_isRunning = true;
_prepareForStop = false;
GET_ENTRY_PTR(FAS_TIMER_MODULE, B)
PREPARE_DIRECTION_PIN(B)
AVR_START_QUEUE(FAS_TIMER_MODULE, B)
break;
#ifdef stepPinStepperC
case channelC:
DISABLE(FAS_TIMER_MODULE, C);
_isRunning = true;
_prepareForStop = false;
GET_ENTRY_PTR(FAS_TIMER_MODULE, C)
PREPARE_DIRECTION_PIN(C)
AVR_START_QUEUE(FAS_TIMER_MODULE, C)
Expand Down

0 comments on commit ea44b43

Please sign in to comment.