code and doc updates

[john-walsh]

For the next set of changes/updates, here is what I'd like to propose:

documentation

Let me know what you'd like to see, in which (maybe new) files?

Do we want a new file for 'history', or should that be in README.md?

If you'd like to suggest a new file layout, I'll go with that, or I can try to develop it.

FYI: I found that:

git log fake6502.c > fake6502_c.log

produces the complete commit history, so we can use that to automate or get a 'history' file started.

code

structs

• I'd like to split (internally) the 'context' struct, into 'cpu' and 'emu' structs, which will then be combined in the 'context' struct.

That would allow the save/restore of each state, separately (useful fro something I have in mind). It's not essential, but would make for better code later on.

• re-instate the 'instructions' counter, again, useful for another change later on.

• uint8_t mem[65536] - replace by a pointer: uint8_t *mem

I can see the use for putting the memory space in the context, but it should be by reference (pointer), rather than by allocation.

As a library, it should not be allocating such memory - it is for the host process to do that (IMHO).

And this change would allow 2 hosted CPU's to share the same memory space - if that's what a user wanted.

For my use case, I already have the 64K allocated, so by having the array in the contact, it is forcing another allocation of memory, which is not going to be used.

source code

• for the option defines (NMOS6502 and CMOS6502), add a logic check, to ensure that one of them has been defined, else give an error.

• move the defines/macros into the header file, so they can be accessed by the host project.

This is something I need to do in my project, as I'm adding some functionality (specific to my project) in my code space, so I need access to them.

• stack pointer (s) - why does it start at 0xfd?

I would have thought that it starts at 0xff.

[omarandlorraine]

As for documentation, I think the most important things to do are:

1. start a changelog. We should move the two notes from Mike Chambers into the changelog and then say what the differences have been since I've forked it. This should be a separate file, maybe CHANGELOG.md or CHANGELOG.rst, up to you.
2. doxygen for externally visible functions maybe

About replacing the uint8_t mem[65536] for uint8_t * mem, this is definitely a step in the right direction. But maybe it should be a void *. Because memory is not always going to be a flat array. There could be bank switching, and there will almost always be memory-mapped hardware and what have you. A void * would let the client code point to whatever it needs. To emulate a Commodore 64 for example, you'd need 64K memory, another 1024 half-bytes of memory, up to 5 different ROMs, lots of support hardware, a state byte to keep track of what's paged in, etc. etc. So that could all be wrapped up in a struct, and the void * would point to that. Hopefully this makes sense.

for the option defines (NMOS6502 and CMOS6502), add a logic check, to ensure that one of them has been defined, else give an error.

Yes. Do you think that there also should be logic to check for the unpermissible situation that both are defined? What about checking for trying to define a CMOS variant with no decimal mode?

move the defines/macros into the header file, so they can be accessed by the host project.

That's fine

stack pointer (s) - why does it start at 0xfd?

This is what the real hardware does, at least from what I understand. There is a comment in reset_6502() explaining this. Basically it gets initialized to $0 or $ff, I can't remember which, but then it accesses the stack and jump vectors in a very similar way to an interrupt (actually, RESET is an interrupt, one that's been rigged to not write anything to the stack). So the new stack pointer is as though it's just pushed PC and PSW, even though the write doesn't actually happen.

[omarandlorraine]

As you're on documentation, I wanted to ask you your opinion on the README. I don't really know what I would suggest, but wanted to see if you see where it falls short? I don't really know much about technical writing.

[john-walsh]

Looks like the code changes you're OK with, so I'll do those.

The 'void*' pointer - is actually what I would rather do, I just didn't know if you knew about that method, and didn't want to over step with any changes.

documentation - I wouldn't say I'm an expert in this area, but I'd like to make it better. And the link you provided (to the changelog project) seams good, if not maybe more than we need.

So I'll see what I come up with.

[john-walsh]

CHANGELOG.md - do you want that to include a dump of the git log history for fake6502.c?

That would give some coverage of the history, and as the project goes forward, better entries can be added.

[omarandlorraine]

CHANGELOG.md - do you want that to include a dump of the git log history for fake6502.c?

I think it would be better for the log history to be higher level. Probably should be much coarser than the commit history, which is easy to find anyway. For example:

• Unit tests for most instructions
• Added context_t pointer argument to interface so your client code can have several emulated CPUs

this kind of high-level view is better for a changelog than a dump of git's commits, much easier to read and understand

[john-walsh]

Yes, agreed - for going forward.

I just would include the git log history, to cover the previous edits that this new file/method does not cover.

[omarandlorraine]

I just would include the git log history, to cover the previous edits that this new file/method does not cover.

I'll see what the pull request is like before I comment further

[omarandlorraine]

ooops accidentally closed it

[john-walsh]

A lot of 'lines' changed, as the context accessing changed.

But otherwise, no real code/functionality changes.

[john-walsh]

Don't merge it in yet, I might have a small update.

[john-walsh]

Ideas for the 3rd update:

1. update test.c to use the void *state_host, for passing the memory through.

So it is better 'example' code.

2. names in the global scope - define/macro/fn...

Are you concerned about there being any name clashes with other peoples code, either in their host source, or with another library that they may be using?

I tend to prefix all my 'global scope' names with something unique. Like a TLA (three letter abbreviation), eg. JDW_readvalue().

So any names that might be of common usage, have less of a chance of clashing with code from/in other modules.

By possible example - in my project, I am looking to also include a Z80 emulator. The one I'm looking at has a global #define A to be some code. That has a very high chance of causing a problem with other source code.

Fake6502 also used to have a global 'a', but since you put the 'registers' into a struct, that has help a lot with this type of problem.

So - would you be interested in some small prefix (eg. f6_, f6502_) or postfix (eg. _6502) for all the global names?

setzero(), setsign() - might be a common name in projects of this nature (CPU based).

3. the compile time options (NES_CPU / NMOS6502 / CMOS6502) :-

a) I prefer to define them in a header file, rather that via the makefile.

This would require a couple of changes in fake6502.h, to #include a couple of outside/host provided files. I have tried this and it works well, and gives another (I think small) benefit to the source code/file structure.

b) I could adapt the code so that they could be run-time selectable.

Again, that may make fake6502 more useful as a library, so the host can choose different CPU configs at run-time.

And keep the option of compiling as a single/set CPU type.

4. adding a couple more 'run code' functions, that the host would then have the option of calling, or not - but at least they are there.

uint8_t call / run6502(context_t *c, uint16_t address)

void exec6502_clk(uint32_t clock_count)

void exec6502_ins(uint32_t instructions_count)

'call' - runs the code at address, and returns the Acc. Then restore the CPU back to it's state before the 'run'.

'exec_*' - runs the emulator up to that many clock ticks or instructions.

So let me know which ones you'd like to do/see/try - we can do them one at a time, if you like, to see how it works.

ps. one more fn(): opcode_replace()

that allows the host code to replace an opcode with it's own fn. I do use this for adding in some 'special' options in my project, kind of like being able to simulate some special hardware mod's, like different IO on/at a given address.

[omarandlorraine]

1. That's a good idea. Even better if test.c can also serve as good example code.
2. Yes a prefix would be useful to reduce the chance of a nameclash. Looking at libz80 for example, they have Z80 at the front of all their functions. I think we could put fake6502_ if it's not too long.
3. Good idea. I would guess the 6502 variant could be an enum, and a member of the context struct.
4. I definitely see the utility of the exec6502_clk and exec6502_ins functions, but I'm not sure about the precise semantics of run6502. Does the latter function run until it hits an rts? what about nested subroutines? What kind of "restore its state" does it need to do?

ps. one more fn(): opcode_replace()
that allows the host code to replace an opcode with it's own fn. I do use this for adding in some 'special' options in my project, kind of like being able to simulate some special hardware mod's, like different IO on/at a given address.

Nifty idea; maybe the unimplemented opcodes should be rigged to call opcode_replace(). So that client code can decide what to do.

So let me know which ones you'd like to do/see/try - we can do them one at a time, if you like, to see how it works.

Yeah I like all your suggestions here. Please can you do one pull request for each one, including your PS.

[omarandlorraine]

Regarding your second bullet point, names like step6502 would probably be better off as fake6502_step. What do you think?

[john-walsh]

I'd be happy with that - just as long as we keep it consistent over everything - defines and macros too?

[omarandlorraine]

Exactly so. Thanks for making this codebase a better one 👍

[john-walsh]

Do you have any thoughts on upper/lower case for fn/macro/define names?

eg:

function names: all_lowercase()
macro names: Upper_FirstLetter()
defines: ALLCAPS

Any guide/preference?

[omarandlorraine]

You've got function names and #defines correct: all_lowercase() and ALLCAPS.

I think that macro names should stay looking like functions, for example, fake6502_set_overflow(c) or whatever. The fact it's a preprocessor macro should be possible to change into a function without breaking too much existing source code.

[john-walsh]

Agreed. And that was going to be another tweak to the code/api - that the read/write memory functions, could also be implemented as macros by the host code.

[john-walsh]

I've done an update to my repo - but didn't do a pull request yet.

Can you check the changes in my repo?

Then, some more changes before the pull request:

1. update all opcode functions (adc, clc, etc...) with the name prefix

2. change the 'zero_calc' names to 'calc_zero' ...

Is that OK so far, and then the 2 additional changes?

And I'll need to update the version/Changelog too.

[omarandlorraine]

That's perfect @john-walsh, thanks

One point about the opcodes, I am not decided but wanted to throw an idea out. Because (we can assume), we don't need to let client code call adc or anything like that, these functions might be better off as static ones. then the names can stay as they are. Might be easier for you and me. What do you think?

[john-walsh]

Let me look at that.

[john-walsh]

I'll leave the 'static' opcodes for the next update.

Just thinking on 'naming conventions' - which do you prefer:

fake6502_set_carry()
fake6502_clear_carry()
fake6502_calc_carry()

or:

fake6502_carry_set()
fake6502_carry_clear()
fake6502_carry_calc()

I prefer the second (object / action). It's a bit more RP (reverse polish notation), but it makes searching for all the 'actions' on an 'object', easier (fake6502_carry_*).

[omarandlorraine]

I think you're right; the second option is better

[john-walsh]

I've updated my repo - have a check and let me know.

I'd do a pull at this point, if all is OK.

[omarandlorraine]

Yep that looks fine, can you do a pull request

[john-walsh]

Next ideas:

1. comments on end of code lines?

Would you rather not have that - I can move them?

2. in test.c, typedef struct test_t, change to: test_fn

3. static opcode fns:

we could do this as a compile time option.

then declare the opcode fns via a macro, that would be either 'static' or not, depending on the compile time option.

so:

void adc(fake6502_context *c) {

would become something like:

FAKE6502_OPCODE(adc) {

or just:

FAKE6502_OP_FN adc(fake6502_context *c) {

[omarandlorraine]

1. I don't really have a strong opinion about that. What benefit is there to moving them? Are any lines too long, as they are?
2. Is this a rename? What for?
3. FAKE6502_OPCODE(adc) { that is a really good idea. The preprocessor will then make all those functions static or not.

[john-walsh]

1 it's more for multi-line style comments (/* */) - they should never be mixed in/on a code line. So it's just a style thing really.

2. Yes, just a rename, to be a better name.

3. OK, I'll do it that way.

[omarandlorraine]

1. I guess you're right. You could change them to single-line comments or move them to a separate line.
2. Go ahead and rename it if you like; it's only local to that one file
3. Splendid, thanks

[john-walsh]

For the fns (point 3), I'd also like to move the opening brace onto a new line.

Is that OK?

[omarandlorraine]

Fine by me 🙂

[john-walsh]

Have a check over these updates.

Turned out there was a name conflict, with test.c, when not 'static' - so I had to prefix the fn name's in test.c.

[omarandlorraine]

Have a check over these updates

Huh? you've got a PR for me?

[john-walsh]

No PR - I'll update in my repo, then when it's good, do the PR.

[omarandlorraine]

Are you on about commit f7da666? that looks fine to me

[john-walsh]

The next updates I think will be the last.

But it's going to require something new - so I have a question for you.

Do you have any ideas on how to do this:

As a source code level library, fake6502 has to both, provide 'stuff' to the host project (I'm thinking of option defines that can be selected), and have the host project provide 'stuff' for it (like the memory accessing fn/macro's).

With only a single header file, this creates a chicken/egg situation.

ie. both the library and the host project, need to know things from the other, and provide input to/for the other.

Have you seen this before, do you have any suggestions?

[omarandlorraine]

With only a single header file, this creates a chicken/egg situation.

I'm not sure how; the library and the host will be compiled separately and then linked together. The #defines that need to be seen by both fake6502 and the host project can be handed around through the build system. For example, the #define YES_I_WANT_STATIC_OPCODE_FUNCTIONS can just be put as -DYES_I_WANT_STATIC_OPCODE_FUNCTIONS in the flags to the compiler.

[john-walsh]

Yes, as the code stands, it's OK.

But as any defines/options get more complex, it would get more involved (like I'm suggesting). I have this in my project (of which fake6502 will be a part) - more complex options at compile time.

And one more personal preference - I'd rather have everything needed to compile the source code, in the source code. I'm not keen on the use of '-D' in the makefile. But, that's my preference.

I'll try to show you an example.

[john-walsh]

• the read/write memory functions, could also be implemented as macros by the host code

so that will require the library (fake6502) including some file that is provided by the host, because the macros need to be known at compile time.

What name could we use for that file?

I would say it needs to be based on the library name, eg: fake6502.???

[omarandlorraine]

the read/write memory functions, could also be implemented as macros by the host code

What is the motive for this? I'm finding it hard to picture a use for it.

[john-walsh]

You've got function names and #defines correct: all_lowercase() and ALLCAPS.

I think that macro names should stay looking like functions, for example, fake6502_set_overflow(c) or whatever. The fact it's a preprocessor macro should be possible to change into a function without breaking too much existing source code.

You thought of it - and I agree, it could be an option.

But I'm trying to be a bit more theoretical - how do 2 sources, both give a take bits to/from each other? What would that source file structure look like?

[omarandlorraine]

fake6502_set_overflow(c) is an easy one, it's implemented by us. We can do a function or a macro without the host caring what choice we've made. There's a point of flexibility in the design of fake6502, because whichever choice we make, the host is getting the functionality from its dependency.

If you know graph theory, the dependency relation between the software components is an acyclic directed graph, roughly meaning the dependency relationship goes only one way.

the read/write memory functions, could also be implemented as macros by the host code

is trickier to do because, as you've noted, it introduces a circular dependency. The directed graph would no longer be acyclic. So far as I've seen, that's not a kind of relationship we see between (well engineered) software projects

[john-walsh]

Yes, I agree with what your saying. I think I'm trying to go beyond the 'norm'.

For a 'standard' library that can be integrated at the object level (by the linker), then you are correct.

I'm aiming for a source code (compile time) level integration, and that could require info passing both ways - between library and host code.

So maybe I'm trying to do too much.

Just on the idea of the host providing the compile time 'defines' in a source file (rather than via the makefile -D), I have seen other libraries (one of the z80 ones I looked at, I think?) including a 'glue.h' header file, that the host code is responsible for providing.

That gives the host source, so involvement/control over the compilation of the library source.

Is that idea OK for you? And what file name would you go for?

I like 'libname.opt' - meaning the setting of any options for the library code.

Could also be 'libname.inc', 'libname.hh' (host header), ...?

So the options available would be documented in/by the library, and provide an example _library.opt file, but it is up to the host to provide the library.opt file - and a new/update of the library would not overwrite the host config/opt file.

[omarandlorraine]

Can you show me the kind of thing you've seen? It sounds like a pain in the rear for not much benefit. But it's of course possible you've seen something I haven't.

[john-walsh]

z80emu

They use 'z80user.h' - and the user can provide macros, so it is interfacing at the compile time level.

And 'z80config.h' - for the user to select config options.

See the comment at the top of z80emu.h

And note the order of inclusion.

'z80config.h' - included first, to set any options.

'z80user.h' - after the z80 library header file, and before the C code, so it has access to defines/types, and can provide macros for the library C fn's.

Other than the naming of the files, that is exactly the same solution that I came to for what I am trying to do with fake6502 - in terms of giving the host code more options.

NB. it also reminds me of a large project I worked on a long time ago - we had an 'options.h' file, that was used as a central dumping ground for all '#define options' across all source code files.

[omarandlorraine]

I don't think we should do it

We need to stay simple and orderly. "Central dumping ground" reeks of Considered Harmful, doesn't it? You're going to have very tight implicit coupling between the software components. And testing just became nearly impossible to do well, especially integration testing.

in terms of giving the host code more options.

More options how exactly? Memory access as a preprocessor directive? Again it seems like the wrong solution. Better define it as a function and let the compiler inline it where it sees fit.

I'll ask again, am I missing something?

[john-walsh]

'options.h' - I wasn't suggesting using that. This just reminded me of that way.

By using the 2 include files like z80emu does, it just gives the host a lot more options - you never know what a host may want to do, so I err on the side of giving them all the options I can.

fake6502 is very simple, and that may be overkill.

So we don't need to go all the way, but I would like to be able to define my 'host project' options in a header file, and there is no way to do that at present.

I'd like to add: _fake6502.opt

which the host renames to' fake6502.opt', and can put their compile options in there.

[omarandlorraine]

Feel free, but I don't think it'll make its way into omarandlorraine/fake6502

[john-walsh]

OK, I'll leave that for my last update.