T1830 MCS51 LJMP address calculation wrong

LJMP needs a 16-bit address.

but addrI08() gets a 8 bit address, 2 addrI08() just get the lowest 8bit.

0x12 LCALL, 0x90 MOV #16bitdata->DPTR all have this bug.

but addrI08() gets a 8 bit address, 2 addrI08() just get the lowest 8bit.

Yes but the 2 bytes are stored in consecutive memory positions, so there are 2 read operations.

arcachofo wrote:

but addrI08() gets a 8 bit address, 2 addrI08() just get the lowest 8bit.

Yes but the 2 bytes are stored in consecutive memory positions, so there are 2 read operations.

Yes 2 read operations are needed. But 2 addrI08() calls just update the lowest 8bit twice.

Ok, I see.

But maybe we could simplify it.
What about this?

Code:


void I51Core::readOperand()
{
    uint8_t addrMode = m_dataEvent.takeFirst();

    if( addrMode & aIMME ){
        if     ( m_cycle == 1 ) m_op0 = m_opAddr = m_pgmData;
        else if( m_cycle == 2 ){                               /// 16 bit addrs
            m_opAddr <<= 8;
            m_opAddr |= m_pgmData;
        }
    }
...
void I51Core::operI08() { m_dataEvent.append( aIMME | aPGM ); }     // m_op0 = data
void I51Core::addrI08() { m_dataEvent.append( aIMME ); }            // m_opAddr = data;


Simpler and faster and we don't need a new enum.

I think it may not work.
Reason:
1. m_opAddr is not cleared before decode. So if addrI08() is the second param decode call, the highest 8bit may use address data from previous instruction.

2.operI08 and addrI08 shouldn't update m_op0 and m_opAddr together. If operI08() is the second param decode call, m_opAddr will be wrongly overwrited.

arcachofo wrote:But maybe we could simplify it.
What about this?

Code:


void I51Core::readOperand()
{
    uint8_t addrMode = m_dataEvent.takeFirst();

    if( addrMode & aIMME ){
        if     ( m_cycle == 1 ) m_op0 = m_opAddr = m_pgmData;
        else if( m_cycle == 2 ){                               /// 16 bit addrs
            m_opAddr <<= 8;
            m_opAddr |= m_pgmData;
        }
    }
...
void I51Core::operI08() { m_dataEvent.append( aIMME | aPGM ); }     // m_op0 = data
void I51Core::addrI08() { m_dataEvent.append( aIMME ); }            // m_opAddr = data;


Simpler and faster and we don't need a new enum.

Nice, now I'm getting the idea, also with this:
https://simulide.forumotion.com/t1651-t1830_operi08-overwrite-m_opaddr#7206