[kromak]

Hello. Recently I wrote this line of code

instruction.process ()

that generates this error message:

Quote

TypeError: process() takes exactly 1 argument (0 given)

This line of code is nothing more than an abstract method of the class Instruction

class Instruction (object):
	__metaclass_ = ABCMeta
		
	def __init__ (self,identification_byte):
		self.identity_byte = identification_byte

	@abstractmethod
	def process (self):
		print ("Identifier Byte: {}".format(self.identity_byte))

that it is implemented in the derived classes that generates the error message as:

class LDAInstruction (Instruction):
	def process (self):
		super.process ()

So I am not sure of what argument he is complaining, as the only argument is the own instance that invokes the method. What am I misunderstanding here ?

[astonecipher]

You have a few issues with that code.

Because python doesn't have interfaces, abstract methods are the equivalent. So, the abstract method is a signature without implementation. That also means, the child class has to implement the abstract method, so process in the LDA class needs the code, not the parent class.

The next issue, and it may be because you are only showing abbreviated code, the parent class takes an argument in the constructor, yet the child class doesn't show a constructor at all? How are you instantiating super?

[kromak]

astonecipher, on 15 October 2019 - 07:52 PM, said:

You have a few issues with that code.

Because python doesn't have interfaces, abstract methods are the equivalent. So, the abstract method is a signature without implementation. That also means, the child class has to implement the abstract method, so process in the LDA class needs the code, not the parent class.

The next issue, and it may be because you are only showing abbreviated code, the parent class takes an argument in the constructor, yet the child class doesn't show a constructor at all? How are you instantiating super?

Hi astonecipher. The reason process is implemented in the parent class is because, until the present moment, I don't know how the process method in each one of the derived class will be. So it makes sense have a single method in the parent one. I expect to be able to leave it unimplemented soon and implement it in the derived classes very soon.

And yes again, there are not child constructors, at least for now. Unlike "process" I didn't even think if there would be constructors for each derived class.

The full code of the "Instruction.py"

from abc import ABCMeta, abstractmethod

class Instruction (object):
	__metaclass_ = ABCMeta
		
	def __init__ (self,identification_byte):
		self.identity_byte = identification_byte

	@abstractmethod
	def process (self):
		print ("Identifier Byte: {}".format(self.identity_byte))

	@abstractmethod
	def process2 (self):
		print ("Identifier Byte2: ", self.identity_byte)
		

class LDAInstruction (Instruction):
	def process (self):
		super.process ()

	def process2 (self):
		super.process()

class SEIInstruction (Instruction):
	def process (self):
		super.process ()

	def process2 (self):
		super.process ()

class CLDInstruction (Instruction):
	def process (self):
		super.process ()

	def process2 (self):
		super.process ()

I commented out the print messages on the process of the parent class and copied them to the process of the child classes, however python then complains about the lack of indented block, pointing to the "@abstractmethod". So I conclude that has to be something on the parent class abstract method.

[astonecipher]

Because of the importance of indention, the abstract method needs a pass in the body.

    @abstractmethod
    def process (self):
        pass
 

    @abstractmethod
    def process2 (self):
        pass

[baavgai]

Ok, so, simplest implementation:

from abc import abstractmethod


class Instruction(object):
    def __init__(self, identification_byte):
        self.identity_byte = identification_byte

    @abstractmethod
    def process(self):
        print("Identifier Byte: {}".format(self.identity_byte))


class LDAInstruction(Instruction):
    def process(self):
        super.process()


LDAInstruction().process()

Result:

Traceback (most recent call last):
  File ".\t.py", line 18, in <module>
    LDAInstruction().process()
TypeError: __init__() missing 1 required positional argument: 'identification_byte'

The error is saying that you've successfully called your parent constructor, but that your child object has failed to implement that constructor. You say LDAInstruction extends Instruction, but where does it implement the constructor?

Let's properly implement the child:

class LDAInstruction(Instruction):
    def __init__(self):
        Instruction.__init__(self, 42)

    def process(self):
        super.process()

Result:

Traceback (most recent call last):
  File ".\t.py", line 21, in <module>
    LDAInstruction().process()
  File ".\t.py", line 18, in process
    super.process()
AttributeError: type object 'super' has no attribute 'process'

Neat. Ah, next bug, then. Final:

from abc import abstractmethod


class Instruction (object):
    def __init__(self, identification_byte):
        self.identity_byte = identification_byte

    @abstractmethod
    def process(self):
        print("Identifier Byte: {}".format(self.identity_byte))


class LDAInstruction(Instruction):
    def __init__(self):
        Instruction.__init__(self, 42)

    def process(self):
        # wrong super.process()
        super().process()


LDAInstruction().process()

Result:

Identifier Byte: 42

As an aside, tabs will only cause you pain and suffering in Python. Indents should always be four spaces. Following this standard will keep things consistent with most other Python programmers and make errors due to white space less common.

Hope this helps.

[kromak]

Ok. I tried to implement the abstract methods in the child classes using pass. I also tried to implement constructors in the child classes like in the above post. Neither worked. The same error message persisted.

Part of the code that might be relevant and belongs to a different class:

instruction = self.instruction_mapping [byte]
			if instruction is None:
				raise Exception ("Instruction not found")
			if (self.pc == 3):
				break
				
			instruction.process ()
			instruction.process2 ()/code] 

[code]self.instruction_mapping = {
			0x78: SEIInstruction,
			0xD8: CLDInstruction,
			0xA9: LDAInstruction
	    }

[baavgai]

kromak, on 18 October 2019 - 07:03 PM, said:

Ok. I tried to implement the abstract methods in the child classes using pass. I also tried to implement constructors in the child classes like in the above post. Neither worked. The same error message persisted.

Right, show the code. Without that, there's really nothing anyone can do you.

kromak, on 18 October 2019 - 07:03 PM, said:

Part of the code that might be relevant

Sorry, selective reveals really aren't that helpful.

I can note that this has some smell:

instruction_mapping = {
    0x78: SEIInstruction,
    0xD8: CLDInstruction,
...

If SEIInstruction is a class, when are you instantiating it? Why would you want to create a new instance each time? Why are you mapping to a class and not something that can do work, like a function or an object?

I have a strong suspicion that you are pointlessly using classes throughout. Given what you have, you'd seem to want something like:

def process_instruction(identification_byte):
    if identification_byte == 0x78:
        process_sei()
    elif identification_byte == 0xD8:
        process_cld()

[kromak]

baavgai, on 19 October 2019 - 01:35 AM, said:

Right, show the code. Without that, there's really nothing anyone can do you.

Ok. File cpu.py:

from rom import ROM
from instruction import LDAInstruction, SEIInstruction, CLDInstruction

class CPU (object):
	def __init__(self, rom_bytes):
		self.registers = []
		self.rom = ROM (rom_bytes)
		self.pc = 0

		self.instruction_mapping = {
			0x78: SEIInstruction,
			0xD8: CLDInstruction,
			0xA9: LDAInstruction
	    }		

	def process_instructions (self):
		for byte in self.rom.data_bytes:
			byte = self.rom.get_byte (self.pc)
			self.pc+=1
			print (byte)
			instruction = self.instruction_mapping [byte]
			if instruction is None:
				raise Exception ("Instruction not found")
			if (self.pc == 3):
				break
				
			instruction.process ()
			instruction.process2 ()

File emulator.py

Quote

import argparse

from cpu import CPU

def main ():

parser = argparse.ArgumentParser (description='NES EMULATOR');
parser.add_argument ('rom_path',metavar='R',type=str,help='path to the rom')
args=parser.parse_args()

with open (args.rom_path, 'rb') as file:
rom_bytes = file.read ()

cpu = CPU(rom_bytes)
cpu.process_instructions ()

if __name__ == '__main__':
main ()

File rom.py

import sys
import struct

HEADER_SIZE = 16
KB_SIZE = 16384

class ROM (object) :
	def __init__ (self, rom_bytes): 
		self.header = rom_bytes [0:HEADER_SIZE]
		self.num_prg_blocks = (self.header [4])
		if (sys.version_info < (3,)):
			self.num_prg_blocks = ord (self.header [4])
		print (type (self.num_prg_blocks))
		self.data_bytes = rom_bytes [HEADER_SIZE:HEADER_SIZE + (16 + KB_SIZE * self.num_prg_blocks)]
		self.total_size = 16 + KB_SIZE * self.num_prg_blocks

	def get_byte (self, pc):
		return	(self.data_bytes [pc])

The instruction.py I posted in the last post.

I can note that this has some smell:

instruction_mapping = {
    0x78: SEIInstruction,
    0xD8: CLDInstruction,
...

If SEIInstruction is a class, when are you instantiating it? Why would you want to create a new instance each time? Why are you mapping to a class and not something that can do work, like a function or an object?

I have a strong suspicion that you are pointlessly using classes throughout. Given what you have, you'd seem to want something like:

def process_instruction(identification_byte):
    if identification_byte == 0x78:
        process_sei()
    elif identification_byte == 0xD8:
        process_cld()

Yes, I am following a tutorial and the guy did it that way. I decided to keep it to mess up with polymorphism, dictionaries in Python. Do you think (anyone's opinion is welcome) it's the case of ditching all of that and do more or less the way you wrote?

[baavgai]

First, I was wrong, if you have no value to pass to a constructor, you can get away without parens. I did a quick test:

class Foo(object):
    def process():
        print("hello from foo")


class Bar(object):
    def process():
        print("hello from bar")


mapping = {
    1: Foo,
    2: Bar
}

mapping[2].process()

I'm still unclear why you'd want a process and process2. Looking at your total code, it still looks like an instruction would want some reference to CPU to do anything.

kromak, on 19 October 2019 - 07:55 PM, said:

ditching all of that and do more or less the way you wrote?

Well, there's more than one way to program anything. It's ultimately up to the programmer as to how to organize things.

I couldn't find the tutorial you referenced, but I did find this repo based on this particular bit of code: self.num_prg_blocks = (self.header [4])

Here's a quick refactor:

class ROM (object):
    HEADER_SIZE = 16
    KB_SIZE = 16384

    def __init__(self, rom_bytes):
        self.header = rom_bytes[0:ROM.HEADER_SIZE]
        self.num_prg_blocks = self.header[4]
        if (sys.version_info < (3,)):
            self.num_prg_blocks = ord(self.num_prg_blocks) # this smells
        # print (type (self.num_prg_blocks))
        # why would you reference rom_bytes if you'd already copied it and why do the math twice?
        # self.data_bytes = rom_bytes [HEADER_SIZE:HEADER_SIZE + (16 + KB_SIZE * self.num_prg_blocks)]
        # self.total_size = 16 + KB_SIZE * self.num_prg_blocks
        self.total_size = ROM.HEADER_SIZE + ROM.KB_SIZE * self.num_prg_blocks
        self.data_bytes = self.header[ROM.HEADER_SIZE:ROM.HEADER_SIZE + self.total_size]

    def get_byte(self, pc):
        return self.data_bytes[pc]


def instruction_generic(cpu, hello):
    print("hello from {}, pc: {}".format(hello, cpu.pc))


def instruction_sei(cpu):
    instruction_generic(cpu, "sei")


def instruction_cld(cpu):
    instruction_generic(cpu, "cld")


def instruction_lda(cpu):
    instruction_generic(cpu, "lda")


def instruction_for_byte(byte):
    print(byte)
    if byte == 0x78:
        return instruction_sei
    elif byte == 0xD8:
        return instruction_cld
    elif byte == 0xA9:
        return instruction_lda
    else:
        return None


class CPU (object):
    def __init__(self, rom_bytes):
        self.registers = []
        self.rom = ROM(rom_bytes)
        self.pc = 0

    def process_instructions(self):
        for byte in self.rom.data_bytes:
            byte = self.rom.get_byte(self.pc)
            self.pc + = 1
            instruction = instruction_for_byte(byte)
            if instruction is None:
                raise Exception("Instruction not found")
            if (self.pc == 3):  # this looks wonky, but we'll leave it
                break
            instruction(self)

The registering the instruction to a mapping is a good idea if everything is spread out and you don't know ahead of time what's getting implemented. However, if that registration is happening local, it seems rather pointless. You might get a little lookup speed, maybe, which you could still do it with functions as above.

Again, it's more about what makes sense to you. Once you have something working, regardless of how messy, you can then start thinking about more clever ways to do things, because you'll have a much better understanding of the problem at that point.

[kromak]

Well, I changed the code to

class Instruction (object):
	__metaclass_ = ABCMeta
		
	def __init__ (self,identification_byte):
		self.identity_byte = identification_byte

	@abstractmethod
	def process (self):
		pass
		
class LDAInstruction (Instruction):
	def process (self):
		print ("Identifier Byte: LDAInstruction:",self.identity_byte)

class SEIInstruction (Instruction):
	def process (self):
		print ("Identifier Byte: SEIInstruction:",self.identity_byte)

class CLDInstruction (Instruction):
	def process (self):
		print ("Identifier Byte: CLDInstruction:",self.identity_byte)

and

def identify_instruction (self, byte):
		if byte==0x78:	
			instruction = LDAInstruction (byte)
		elif byte==0xD8:
			instruction = SEIInstruction (byte)
		elif byte==0xA9:
			instruction = CLDInstruction (byte)
		return instruction
		

	def process_instructions (self):
		for byte in self.rom.data_bytes:
			byte = self.rom.get_byte (self.pc)
			self.pc+=1
			print (byte)
			instruction = self.identify_instruction (byte)
			if instruction is None:
				raise Exception ("Instruction not found")
			instruction.process ()
			if (self.pc == 3):
				break

it's working now. On Python 3.2. But on 2.6 I receive

Quote

File "/home/leopoldo/cpu.py", line 17, in identify_instruction
return instruction
UnboundLocalError: local variable 'instruction' referenced before assignment

self.num_prg_blocks = ord(self.num_prg_blocks) # this smells

Well, it's the only way I was able to make it run on both Python simultaneously

 # why would you reference rom_bytes if you'd already copied it and why do the math twice?

I do not remember, but I will ditch "total_size", as I do not see use for it, so does not matter.

[baavgai]

This looked moderately interesting. You're essentially emulating a 6502 CPU. I found some fun starter docs here. And, full instruction set docs here and here.

It's rather a lot of work, though each instruction is pretty simple. You'd really want to test each and make sure all the registers are behaving as expected.

This was my start up at such a project, using that first link for a three line test program:

def exec_lda_immediate():
    def f(cpu):
        cpu.reg_a = cpu.mem[cpu.reg_pc]
        cpu.reg_pc += 1
    return (0xA9, f)


def exec_tax():
    def f(cpu):
        cpu.reg_x = cpu.reg_a
    return (0xAA, f)


def exec_brk():
    def f(cpu):
        pass
    return (0x00, f)


class CPU6502(object):
    MASK_N, MASK_V, MASK_B = 0x80, 0x40, 0x10
    MASK_D, MASK_I, MASK_Z, MASK_C = 0x08, 0x04, 0x02, 0x01

    def __init__(self, mem=[0 for _ in range(128)]):
        self.mem = mem[:]
        self.reg_a = 0  # 8-bit accumulator register (A)
        self.reg_x = 1  # 8-bit index registers
        self.reg_y = 1  # 8-bit index registers
        self.reg_sp = 1  # 8-bit stack pointer (S)
        self.reg_pc = 0  # 16-bit program counter (PC)
        self.reg_status = 0  # 7 processor status flag bits (P)
        self.all_exec = dict(f() for f in (
            exec_lda_immediate, exec_tax,
            exec_brk))

    def __str__(self):
        xs = [{True: 1, False: 0}[(x & self.reg_status) != 0] for x in (CPU6502.MASK_N, CPU6502.MASK_V, 0, CPU6502.MASK_B, CPU6502.MASK_D, CPU6502.MASK_I, CPU6502.MASK_Z, CPU6502.MASK_C)]
        pf = ''.join(('-', c)[i] for (c, i) in zip("NV-BDIZC", xs))
        return f"A:${self.reg_a:02X} X:${self.reg_x:02X} Y:${self.reg_y:02X} SP:${self.reg_sp:02X} PC:${self.reg_pc:04X} PF:{pf}"
        # A=$c0 X=$00 Y=$00 SP=$ff PC=$0602 NV-BDIZC 10110000

    def is_done(self):
        return self.reg_pc >= len(self.mem) or self.mem[self.reg_pc] == 0x00

    def step(self):  # execute one instruction
        if not self.is_done():
            instruction = self.mem[self.reg_pc]
            self.reg_pc += 1
            if instruction in self.all_exec:
                self.all_exec[instruction](self)

    def run(self, trace=False):  # execute
        def tr():
            if trace:
                print(self)
        tr()
        while not self.is_done():
            self.step()
            tr()


def asm_lda_immediate(byte):
    return [0xA9, byte]


def asm_tax():
    return [0xAA]


def asm_brk():
    return [0x00]


def main():
    mem = asm_lda_immediate(0xc0) + asm_tax() + asm_brk()
    print("pgm: " + ' '.join(f'{x:02X}' for x in mem))

    cpu = CPU6502(mem)
    cpu.run(True)


main()

Result:

pgm: A9 C0 AA 00
A:$00 X:$01 Y:$01 SP:$01 PC:$0000 PF:--------
A:$C0 X:$01 Y:$01 SP:$01 PC:$0002 PF:--------
A:$C0 X:$C0 Y:$01 SP:$01 PC:$0003 PF:--------

Looks like it's behaving. Like I said, a lot of work to get something useful. Good luck.

[Histiogge]

Totally agree with baavgai, I'd do the same.