Retrochallenge: Refactorchallenge

In software, as in most things, you seldom get it right on the first try.

(Ok, yeah, it’s July 20, the anniversary of a guy named Neil getting something right on the first try. But for the rest of us…)

In my last post, I had basic file and directory reading and writing working for my Retrochallenge project. If I’d had any sense, I would’ve continued on from there, added file writes, and been mostly done right now. Instead, I decided to refactor.

First, a warning: this entry features more than the usual amount of 6502 assembly for this blog, including a lot of what I’ve decided are bad ways to implement ideas. If you don’t know the difference between indirect indexed addressing and indexed indirect addressing – well, I have to look it up every time, too.

My code worked, but I found it aesthetically unappealing. In particular, I had some function table dispatch stuff that was just ugly. The idea is that I have a routine like getc, which takes a logical device number for an argument. It looks up that device number in my device table to find a pointer to the “real” getc implementation for that physical device, along with a channel number that I’m using to have multiple files open at once.

For example, the command channel for my SD card is logical device 1. It has a channel number of 0 (which tells the Arduino on the other side of the port that we’re sending a command instead of writing to a file), and there are device specific routines like SD_GETC and SD_PUTC which know how to talk to the Arduino over the parallel port.

My first attempt at implementing this in 6502 assembly language was less than elegant. I’m not really a sophisticated assembly hacker, so sometimes I overlook features that would make my life easier. I also tend to shoehorn high-level-language constructs into places they don’t really belong.

Trying to avoid that bad habit, and outsmarting myself, I implemented my device table as a bunch of arrays indexed by the device number – one array for the channel numbers for each logical device, one for the getc pointers, one for the putc pointers, and so on. I tried to do everything in one function, and that led me to this implementation:

Well, it worked, but the array indexing wasn’t as elegant as I intended. That’s because GETC_TAB is an array of 16-bit pointers and CHANNEL_TAB is an array of 8-bit numbers. So I have to shift the device number to get one offset, and save it or shift it back to get the other. Don’t even ask why I decided to store the device number on the stack instead of just rotating it back to the original value, because I can’t tell you.

I thought about splitting GETC_TAB (and PUTC_TAB, INIT_TAB, etc.) into separate arrays for the low and high bytes to simplify the indexing, but the ugliness of what I was doing was getting to me. Instead, I broke down and rearranged the device table into something that looked more like an array of C-style device structs. I figured I could generate a pointer to the struct for a particular device and save that somewhere that I could use it for indirect references to the function pointers. I also split things into two subroutines, set_device and getc, so that I didn’t have to juggle all the parameters in registers at once.

This is what I meant when I was talking about high-level-languages before. It may be more C-like, but I’m willing to say that this is objectively worse than the previous version. Then I noticed a feature of 6502 assembly language that I’d overlooked before. My P:65 computer uses a 65C02 CPU, which adds a few instructions to the original 6502. It also adds new addressing modes to existing instructions, such as an Absolute Indexed Indirect version of the JMP opcode.

Talk about a mouthful! What “Absolute Indexed Indirect” means is that it takes an address, adds the X register to it, and looks in that location for the address to jump to. It’s perfect for doing these structure table jumps. Once I rearranged the math, I realized I didn’t even need an explicit pointer to the DEVENTRY – I just needed to calculate the offset from the start of the table, which is the logical device number multiplied by the size of DEVENTRY struct. My code simplified to this:

Wow. I wish I’d written it that way in the first place! It just goes to show how important the refactoring process can be, especially when you’re learning your way around a language. It’s okay for your first solution to be hideous and overcomplicated, but you shouldn’t be afraid to go back and improve things when you’ve figured out a better way – especially in a hobby project.

Now, okay, a timed challenge like Retrochallenge may not be the best time to get caught up in refactoring, but I do think this will pay off over the next couple days as I try to call some of these routines from CC65‘s libc. I’ll be crossing my fingers as I get back to work.