Some hints and questions

[Dudeplayz]

Hello Uri,
Dario here, we have written per mail some weeks ago. I had now the time to take a look at the project. I have found some small questions and maybe hints I want to discuss here:

1 - Would it be useful to allow multiple interrupt hooks for the same address? In this case, you can, for example, attach a logger for specific addresses or something like that.

2 - In the cpu.ts the hook is called and checked for a boolean. I have seen that this is used for gpio and if the hook returns true, the write is not saved into memory. Can you tell me, why you have designed it this way?

3 - Some places in the code use number, but you have declared some types for smaller ones, which are mapped to number. I think this places in code should be refactored to use the correct reference instead of number to make it clear which type is expected to make it more compatible with future additions and environments. For example here the number can be replaced with u16 (uint16):

avr8js/src/cpu/cpu.ts

Lines 81 to 83 in f34f825

	set SP(value: number) {
	this.dataView.setUint16(93, value, true);
	}

4 - A similar case is, that the return types are not declared explicitly, which is not necessary in typescript but makes it more clear what is expected and new contributors can understand it better.

5 -

avr8js/src/cpu/cpu.ts

Lines 89 to 91 in f34f825

	get interruptsEnabled() {
	return this.SREG & 0x80 ? true : false;
	}

Could be refactored to:

get interruptsEnabled() {
    return !!(this.SREG & 0x80);
}

(Just a hint of my IDE don't know if it speed up anything)

6 - Code points like:

avr8js/src/cpu/instruction.ts

Line 781 in f34f825

cpu.cycles++;

Which are called very often could be refactored to ++cpu.cycles to cause a slightly faster operation. See StackOverflow: old link, but found also some newer articles about it.

7 - The next question is, how works the command decoding at the moment? I've seen the big if-else block and my mind tells me to refactor it 😄, but I think I've seen another issue about this already? Something like a look-up table would be much faster, but in this case, I think the whole decoding must be refactored. For the moment a smaller speed up would be to take some heuristics and reorder the if-else cases so that the commands which are called more often are "found" faster on the top.
8 -

avr8js/src/peripherals/gpio.ts

Line 99 in f34f825

this.writeGpio(value & portValue, oldValue & oldValue);

Has the oldValue & oldValue some effects? It looks for me like a copy&paste with forgotten refactor 😄.

9 -

avr8js/src/peripherals/usart.ts

Lines 116 to 133 in f34f825

	get bitsPerChar() {
	const ucsz =
	((this.cpu.data[this.config.UCSRA] & (UCSRC_UCSZ1 | UCSRC_UCSZ0)) >> 1) |
	(this.cpu.data[this.config.UCSRB] & UCSRB_UCSZ2);
	switch (ucsz) {
	case 0:
	return 5;
	case 1:
	return 6;
	case 2:
	return 7;
	case 3:
	return 8;
	default: // 4..6 are reserved
	case 7:
	return 9;
	}
	}

Is it correct that case 7 applies to the default case (4 .. 6)? Haven't dug through the code or the sheets to know the effect of this, so it's only a question.

So thank you for reading until this point. If something is helpful it can be put in separate issues 😀.

Have a nice day!

[urish]

Wow Dario, this is a lot of detailed feedback, thank you for taking the time to look at this and share your feedback!

I will go over it again shortly and write detailed answers to your questions

[urish]

Hi Dario,

Answering some of your questions here:

1 - Would it be useful to allow multiple interrupt hooks for the same address? In this case, you can, for example, attach a logger for specific addresses or something like that.

Are you talking about the writeHooks? It can be done by extending the CPU class and overriding writeData() with a different implementation. So far, I haven't seen a need to implement this functionality in the base CPU class.

2 - In the cpu.ts the hook is called and checked for a boolean. I have seen that this is used for gpio and if the hook returns true, the write is not saved into memory. Can you tell me, why you have designed it this way?

In some cases, writing to a certain register triggers an action that may result in a different output value. GPIO is one example: when a GPIO pin is in OUTPUT mode, writing 1 to any of the bits in the PIN register, actually toggles the state of the respective pin (and thus also the matching bit of the PORT) register. You can read more about this in section 14.2.2 / page 85 of the datasheet

3 - Some places in the code use number, but you have declared some types for smaller ones, which are mapped to number. I think this places in code should be refactored to use the correct reference instead of number to make it clear which type is expected to make it more compatible with future additions and environments.
That's true, it will probably become relevant if we wanted to experiment compiling the code using AssemblyScript (as you suggested in #35). Feel free to send a pull-request to fix these places.

4 - A similar case is, that the return types are not declared explicitly, which is not necessary in typescript but makes it more clear what is expected and new contributors can understand it better.

I basically refrain from explicitly adding return type annotations, as most editors let you see the inferred return type when you mouse your mouse over the function name:

However, it might be required for AssemblyScript, and if that is the case then I wouldn't mind adding them.

5/6 - I don't think these make any noticeable difference. One thing that we should probably do is to see how this is translated to v8 bytecode, this could give us a hint whether there is a real difference between these alternatives.

7 - The next question is, how works the command decoding at the moment?

Yes, it's a giant if-else statement. The function might be probably too big for turbofan to optimize, and using a lookup table will probably speed up things. I started some work on it inside the benchmark folder, but the code there has to be updated after the changes introduced by #19.

8 -Has the oldValue & oldValue some effects? It looks for me like a copy&paste with forgotten refactor 😄.

Yes, it seems so. I guess it should have been oldValue & oldValue, so we need to come up with a failing test case and fix this. Thanks for spotting it! Can please please open a separate issue for that?

9 - Is it correct that case 7 applies to the default case (4 .. 6)? Haven't dug through the code or the sheets to know the effect of this, so it's only a question.

Right now we don't even implement non-standard byte sizes in the USART simulation, so it always behaves as if bitsPerChar is 8. The datasheet does not seem to specify the behavior for cases 4 .. 6, it only says "reserved", so I guess the only way to know is to try and see with real hardware and a scope. However, I suspect that non 8-bit per char are very rarely used, so I wouldn't spend time figuring this out.

I hope that my answers are useful :)

[urish]

Update regarding item number 7: I have updated the benchmark code to work again. You can generate the lookup table by running npm run benchmark:prepare

[Dudeplayz]

Thanks for the detailed explanation. I will take a closer look at it again and will check out the references and changes you have made.

[urish]

Seems like the questions have been answered, so closing this issue