Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ap2_dsk.cpp/h: clean up constants #13021

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+105 −104
Open

ap2_dsk.cpp/h: clean up constants #13021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 100 additions & 96 deletions src/lib/formats/ap2_dsk.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ int a2_13sect_format::identify(util::random_read &io, uint32_t form_factor, cons
if (io.length(size))
return 0;

if (size != APPLE2_STD_TRACK_COUNT * 13 * 256)
if (size != APPLE2_STD_TRACK_COUNT * 13 * APPLE2_SECTOR_SIZE)
return 0;

return FIFID_SIZE;
Expand All @@ -49,11 +49,11 @@ bool a2_13sect_format::load(util::random_read &io, uint32_t form_factor, const s

image.set_form_variant(floppy_image::FF_525, floppy_image::SSSD);

int tracks = size / 13 / 256;
int tracks = size / 13 / APPLE2_SECTOR_SIZE;

for(int track = 0; track < tracks; track++) {
std::vector<uint32_t> track_data;
uint8_t sector_data[256 * 13];
uint8_t sector_data[APPLE2_SECTOR_SIZE * 13];

auto const [err, actual] = read_at(
io, track * sizeof sector_data, sector_data, sizeof sector_data);
Expand Down Expand Up @@ -89,7 +89,7 @@ bool a2_13sect_format::load(util::random_read &io, uint32_t form_factor, const s
pval = nval;
};

const uint8_t *sdata = sector_data + 256 * sector;
const uint8_t *sdata = sector_data + APPLE2_SECTOR_SIZE * sector;

// write 154 bytes encoding bits 2-0
write_data_byte(sdata[255] & 7);
Expand Down Expand Up @@ -218,16 +218,17 @@ int a2_16sect_format::identify(util::random_read &io, uint32_t form_factor, cons
if (io.length(size))
return 0;

//uint32_t expected_size = 35 * 16 * 256;
uint32_t expected_size = APPLE2_TRACK_COUNT * 16 * 256;

// check standard size plus some oddball sizes in our softlist
if ((size != expected_size) && (size != 35 * 16 * 256) && (size != 143403) && (size != 143363) && (size != 143358))
if (
size != APPLE2_TRACK_COUNT * 16 * APPLE2_SECTOR_SIZE
&& size != APPLE2_STD_TRACK_COUNT * 16 * APPLE2_SECTOR_SIZE
&& size != 143403 && size != 143363 && size != 143358
)
{
return 0;
}

uint8_t sector_data[256*2];
uint8_t sector_data[APPLE2_SECTOR_SIZE*2];
static const unsigned char pascal_block1[4] = { 0x08, 0xa5, 0x0f, 0x29 };
static const unsigned char pascal2_block1[4] = { 0xff, 0xa2, 0x00, 0x8e };
static const unsigned char dos33_block1[4] = { 0xa2, 0x02, 0x8e, 0x52 };
Expand Down Expand Up @@ -304,18 +305,18 @@ bool a2_16sect_format::load(util::random_read &io, uint32_t form_factor, const s

image.set_form_variant(floppy_image::FF_525, floppy_image::SSSD);

int tracks = (size == (40 * 16 * 256)) ? 40 : 35;
int tracks = (size == (APPLE2_TRACK_COUNT * 16 * APPLE2_SECTOR_SIZE)) ? APPLE2_TRACK_COUNT : APPLE2_STD_TRACK_COUNT;

int fpos = 0;
for(int track=0; track < tracks; track++) {
std::vector<uint32_t> track_data;
uint8_t sector_data[256*16];
uint8_t sector_data[APPLE2_SECTOR_SIZE*16];

auto const [err, actual] = read_at(io, fpos, sector_data, sizeof sector_data);
if (err || actual != sizeof sector_data)
return false;

fpos += 256*16;
fpos += APPLE2_SECTOR_SIZE*16;
for(int i=0; i<49; i++)
raw_w(track_data, 10, 0x3fc);
for(int i=0; i<16; i++) {
Expand All @@ -330,7 +331,7 @@ bool a2_16sect_format::load(util::random_read &io, uint32_t form_factor, const s
sector = dos_skewing[i];
}

const uint8_t *sdata = sector_data + 256 * sector;
const uint8_t *sdata = sector_data + APPLE2_SECTOR_SIZE * sector;
for(int j=0; j<20; j++)
raw_w(track_data, 10, 0x3fc);
raw_w(track_data, 8, 0xff);
Expand Down Expand Up @@ -393,34 +394,37 @@ uint8_t a2_16sect_format::gb(const std::vector<bool> &buf, int &pos, int &wrap)
return v;
}

//#define VERBOSE_SAVE

bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint32_t> &variants, const floppy_image &image) const
{
int g_tracks, g_heads;
int visualgrid[16][APPLE2_TRACK_COUNT]; // visualizer grid, cleared/initialized below

// lenient addr check: if unset, only accept an addr mark if the checksum was good
// if set, accept an addr mark if the track and sector values are both sane
#undef LENIENT_ADDR_CHECK
// if set, use the old, not as robust logic for choosing which copy of a decoded sector to write
// to the resulting image if the sector has a bad checksum and/or postamble
#undef USE_OLD_BEST_SECTOR_PRIORITY
// nothing found
#define NOTFOUND 0
// address mark was found
#define ADDRFOUND 1
// address checksum is good
#define ADDRGOOD 2
// data mark was found (requires addrfound and sane values)
#define DATAFOUND 4
// data checksum is good
#define DATAGOOD 8
// data postamble is good
#define DATAPOST 16
constexpr bool VERBOSE_SAVE = false;

// if false, only accept an addr mark if the checksum was good
// if true, accept an addr mark if the track and sector values are both sane
constexpr bool LENIENT_ADDR_CHECK = false;

// if true, use the old, not as robust logic for choosing which copy of a decoded sector to write
// to the resulting image if the sector has a bad checksum and/or postamble
constexpr bool USE_OLD_BEST_SECTOR_PRIORITY = false;

// nothing found
constexpr int NOTFOUND = 0;
// address mark was found
constexpr int ADDRFOUND = 1;
// address checksum is good
constexpr int ADDRGOOD = 2;
// data mark was found (requires addrfound and sane values)
constexpr int DATAFOUND = 4;
// data checksum is good
constexpr int DATAGOOD = 8;
// data postamble is good
constexpr int DATAPOST = 16;

for (auto & elem : visualgrid) {
for (int j = 0; j < APPLE2_TRACK_COUNT; j++) {
elem[j] = 0;
elem[j] = NOTFOUND;
}
}
image.get_actual_geometry(g_tracks, g_heads);
Expand All @@ -430,16 +434,16 @@ bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint3
int pos_data = 0;

for(int track=0; track < g_tracks; track++) {
uint8_t sectdata[(256)*16];
uint8_t sectdata[APPLE2_SECTOR_SIZE*16];
memset(sectdata, 0, sizeof(sectdata));
int nsect = 16;
#ifdef VERBOSE_SAVE
fprintf(stderr,"DEBUG: a2_16sect_format::save() about to generate bitstream from track %d...", track);
#endif
if constexpr (VERBOSE_SAVE) {
fprintf(stderr,"DEBUG: a2_16sect_format::save() about to generate bitstream from track %d...", track);
}
auto buf = generate_bitstream_from_track(track, head, 3915, image);
#ifdef VERBOSE_SAVE
fprintf(stderr,"done.\n");
#endif
if constexpr (VERBOSE_SAVE) {
fprintf(stderr,"done.\n");
}
int pos = 0;
int wrap = 0;
int hb = 0;
Expand Down Expand Up @@ -471,19 +475,17 @@ bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint3
uint8_t tr = gcr4_decode(h[2],h[3]);
uint8_t se = gcr4_decode(h[4],h[5]);
uint8_t chk = gcr4_decode(h[6],h[7]);
#ifdef VERBOSE_SAVE
uint32_t post = get_u24be(&h[8]);
printf("Address Mark:\tVolume %d, Track %d, Sector %2d, Checksum %02X: %s, Postamble %03X: %s\n", vl, tr, se, chk, (chk ^ vl ^ tr ^ se)==0?"OK":"BAD", post, (post&0xFFFF00)==0xDEAA00?"OK":"BAD");
#endif
if constexpr (VERBOSE_SAVE) {
uint32_t post = get_u24be(&h[8]);
printf("Address Mark:\tVolume %d, Track %d, Sector %2d, Checksum %02X: %s, Postamble %03X: %s\n",
vl, tr, se, chk, (chk ^ vl ^ tr ^ se)==0?"OK":"BAD", post, (post&0xFFFF00)==0xDEAA00?"OK":"BAD");
Comment on lines +480 to +481
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use an additional indentation level for statement continuations vs scope levels, e.g.:

    if (x)
    {
        f( // indented one level because scope depth increased
                a, // two levels for continuation
                b);
        g();
    }

}
// sanity check
if (tr == track && se < nsect) {
visualgrid[se][track] |= ADDRFOUND;
visualgrid[se][track] |= ((chk ^ vl ^ tr ^ se)==0)?ADDRGOOD:0;
#ifdef LENIENT_ADDR_CHECK
if ((visualgrid[se][track] & ADDRFOUND) == ADDRFOUND) {
#else
if ((visualgrid[se][track] & ADDRGOOD) == ADDRGOOD) {
#endif
visualgrid[se][track] |= ADDRFOUND;
visualgrid[se][track] |= ((chk ^ vl ^ tr ^ se)==0)?ADDRGOOD:0;

if (visualgrid[se][track] & (LENIENT_ADDR_CHECK ? ADDRFOUND : ADDRGOOD)) {
int opos = pos;
int owrap = wrap;
hb = 0;
Expand All @@ -509,11 +511,11 @@ bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint3

if (m_prodos_order)
{
dest = sectdata+(256)*prodos_skewing[se];
dest = sectdata+APPLE2_SECTOR_SIZE*prodos_skewing[se];
}
else
{
dest = sectdata+(256)*dos_skewing[se];
dest = sectdata+APPLE2_SECTOR_SIZE*dos_skewing[se];
}

// first read in sector and decode to 6bit form
Expand Down Expand Up @@ -542,37 +544,39 @@ bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint3
// but only write it if the bitfield of the track shows datagood is NOT set.
// if it is set we don't want to overwrite a guaranteed good read with a bad one
// if past read had a bad checksum or bad postamble...
#ifndef USE_OLD_BEST_SECTOR_PRIORITY
if (((visualgrid[se][track]&DATAGOOD)==0)||((visualgrid[se][track]&DATAPOST)==0)) {
// if the current read is good, and postamble is good, write it in, no matter what.
// if the current read is good and the current postamble is bad, write it in unless the postamble was good before
// if the current read is bad and the current postamble is good and the previous read had neither good, write it in
// if the current read isn't good and neither is the postamble but nothing better
// has been written before, write it anyway.
if ( ((data[0x156] == c) && (dpost&0xFFFF00)==0xDEAA00) ||
(((data[0x156] == c) && (dpost&0xFFFF00)!=0xDEAA00) && ((visualgrid[se][track]&DATAPOST)==0)) ||
(((data[0x156] != c) && (dpost&0xFFFF00)==0xDEAA00) && (((visualgrid[se][track]&DATAGOOD)==0)&&(visualgrid[se][track]&DATAPOST)==0)) ||
(((data[0x156] != c) && (dpost&0xFFFF00)!=0xDEAA00) && (((visualgrid[se][track]&DATAGOOD)==0)&&(visualgrid[se][track]&DATAPOST)==0))
) {
if constexpr (USE_OLD_BEST_SECTOR_PRIORITY) {
if ((visualgrid[se][track]&DATAGOOD)==0) {
for(int i=0x56; i<0x156; i++) {
uint8_t dv = data[i];
*dest++ = dv;
}
}
}
#else
if ((visualgrid[se][track]&DATAGOOD)==0) {
for(int i=0x56; i<0x156; i++) {
uint8_t dv = data[i];
*dest++ = dv;
else {
Comment on lines 554 to +555
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is already inconsistent with K&R vs Allman formatting, but I don’t think any of it uses this style:

    }
    else {

It’s all either Allman:

    }
    else
    {

or K&R:

    } else {

You should use the predominant style of the file.

if (((visualgrid[se][track]&DATAGOOD)==0)||((visualgrid[se][track]&DATAPOST)==0)) {
// if the current read is good, and postamble is good, write it in, no matter what.
// if the current read is good and the current postamble is bad, write it in unless the postamble was good before
// if the current read is bad and the current postamble is good and the previous read had neither good, write it in
// if the current read isn't good and neither is the postamble but nothing better
// has been written before, write it anyway.
if ( ((data[0x156] == c) && (dpost&0xFFFF00)==0xDEAA00) ||
(((data[0x156] == c) && (dpost&0xFFFF00)!=0xDEAA00) && ((visualgrid[se][track]&DATAPOST)==0)) ||
(((data[0x156] != c) && (dpost&0xFFFF00)==0xDEAA00) && (((visualgrid[se][track]&DATAGOOD)==0)&&(visualgrid[se][track]&DATAPOST)==0)) ||
(((data[0x156] != c) && (dpost&0xFFFF00)!=0xDEAA00) && (((visualgrid[se][track]&DATAGOOD)==0)&&(visualgrid[se][track]&DATAPOST)==0))
) {
for(int i=0x56; i<0x156; i++) {
uint8_t dv = data[i];
*dest++ = dv;
}
}
}
}
#endif
// do some checking
#ifdef VERBOSE_SAVE
if ((data[0x156] != c) || (dpost&0xFFFF00)!=0xDEAA00)
fprintf(stderr,"Data Mark:\tChecksum xpctd %d found %d: %s, Postamble %03X: %s\n", data[0x156], c, (data[0x156]==c)?"OK":"BAD", dpost, (dpost&0xFFFF00)==0xDEAA00?"OK":"BAD");
#endif
if constexpr (VERBOSE_SAVE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to prepend these with constexpr, as MAME has the appropriate compiler flags set to handle effective if (0) and if (1) situations without warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but is there any harm in it? I think it's a useful reminder that it's supposed to be a compile-time selection.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s not what if constexpr is for. The purpose of if constexpr is for when one branch is malformed when the expression is false. For example see use in src.emu/device.h:

    template <class DeviceClass>
    DeviceClass *subdevice(std::string_view tag) const
    {
        device_t *const found = subdevice(tag);
        if constexpr (std::is_base_of_v<device_t, DeviceClass>)
        {
            return downcast<DeviceClass *>(found);
        }
        else
        {
            auto const result = dynamic_cast<DeviceClass *>(found);
#if defined(MAME_DEBUG)
            if (found && !result)
                report_bad_device_cast(found, typeid(device_t), typeid(DeviceClass));
#endif
            return result;
        }
    }

The code can use the downcast helper if DeviceClass is derived from device_t, but this is invalid otherwise (e.g. if it is derived from device_interface but not device_t).

Or consider this code in src/lib/util/delegate.h where it needs to make a decision on whether it can cast the call operator to a usable function type or it needs to wrap it in a lambda to convert the return value:

    template <typename T>
    static functoid_setter make_functoid_setter()
    {
        if constexpr (matching_non_const_call(&unwrapped_functoid_t<T>::operator()))
        {
            return
                    [] (delegate &obj)
                    {
                        obj.basetype::operator=(
                                basetype(
                                    static_cast<ReturnType (unwrapped_functoid_t<T>::*)(Params...)>(&unwrapped_functoid_t<T>::operator()),
                                    obj.unwrap_functoid<T>()));
                    };
        }
        else if constexpr (matching_const_call(&unwrapped_functoid_t<T>::operator()))
        {
            return
                    [] (delegate &obj)
                    {
                        obj.basetype::operator=(
                                basetype(
                                    static_cast<ReturnType (unwrapped_functoid_t<T>::*)(Params...) const>(&unwrapped_functoid_t<T>::operator()),
                                    obj.unwrap_functoid<T>()));
                    };
        }
        else
        {
            return
                    [] (delegate &obj)
                    {
                        obj.basetype::operator=(
                                basetype(
                                    [] (unwrapped_functoid_t<T> &f, Params... args) { return ReturnType(f(std::forward<Params>(args)...)); },
                                    obj.unwrap_functoid<T>()));
                    };
        }
    }

Depending on the signature of the call operator, one or two of the branches will be malformed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can find plenty of examples of testing compile-time constant expressions with regular if, e.g. in src/devices/cpu/e132xs/e132xsop.hxx:

template <hyperstone_device::reg_bank DST_GLOBAL, hyperstone_device::reg_bank SRC_GLOBAL>
void hyperstone_device::hyperstone_and()
{
        check_delay_PC();

        const uint32_t dst_code = DST_GLOBAL ? DST_CODE : ((DST_CODE + GET_FP) & 0x3f);
        const uint32_t sreg = SRC_GLOBAL ? m_core->global_regs[SRC_CODE] : m_core->local_regs[(SRC_CODE + GET_FP) & 0x3f];
        const uint32_t dreg = (DST_GLOBAL ? m_core->global_regs : m_core->local_regs)[dst_code] & sreg;

        if (dreg == 0)
                SR |= Z_MASK;
        else
                SR &= ~Z_MASK;

        if (DST_GLOBAL)
                set_global_register(dst_code, dreg);
        else
                m_core->local_regs[dst_code] = dreg;

        m_core->icount -= m_core->clock_cycles_1;
}

if ((data[0x156] != c) || (dpost&0xFFFF00)!=0xDEAA00)
fprintf(stderr,"Data Mark:\tChecksum xpctd %d found %d: %s, Postamble %03X: %s\n",
data[0x156], c, (data[0x156]==c)?"OK":"BAD", dpost, (dpost&0xFFFF00)==0xDEAA00?"OK":"BAD");
}
if (data[0x156] == c) visualgrid[se][track] |= DATAGOOD;
if ((dpost&0xFFFF00)==0xDEAA00) visualgrid[se][track] |= DATAPOST;
} else if ((hb == 4)&&(dosver == 1)) {
Expand All @@ -590,33 +594,33 @@ bool a2_16sect_format::save(util::random_read_write &io, const std::vector<uint3
}
for(int i=0; i<nsect; i++) {
//if(nsect>0) printf("t%d,", track);
uint8_t const *const data = sectdata + (256)*i;
auto const [err, actual] = write_at(io, pos_data, data, 256);
if (err || actual != 256)
uint8_t const *const data = sectdata + APPLE2_SECTOR_SIZE*i;
auto const [err, actual] = write_at(io, pos_data, data, APPLE2_SECTOR_SIZE);
if (err || actual != APPLE2_SECTOR_SIZE)
return false;
pos_data += 256;
pos_data += APPLE2_SECTOR_SIZE;
}
//printf("\n");
}
// display a little table of which sectors decoded ok
#ifdef VERBOSE_SAVE
int total_good = 0;
for (int j = 0; j < APPLE2_TRACK_COUNT; j++) {
printf("T%2d: ",j);
for (int i = 0; i < 16; i++) {
if (visualgrid[i][j] == NOTFOUND) printf("-NF- ");
else {
if (visualgrid[i][j] & ADDRFOUND) printf("a"); else printf(" ");
if (visualgrid[i][j] & ADDRGOOD) printf("A"); else printf(" ");
if (visualgrid[i][j] & DATAFOUND) printf("d"); else printf(" ");
if (visualgrid[i][j] & DATAGOOD) { printf("D"); total_good++; } else printf(" ");
if (visualgrid[i][j] & DATAPOST) printf("."); else printf(" ");
if constexpr (VERBOSE_SAVE) {
int total_good = 0;
for (int j = 0; j < APPLE2_TRACK_COUNT; j++) {
printf("T%2d: ",j);
for (int i = 0; i < 16; i++) {
if (visualgrid[i][j] == NOTFOUND) printf("-NF- ");
else {
if (visualgrid[i][j] & ADDRFOUND) printf("a"); else printf(" ");
if (visualgrid[i][j] & ADDRGOOD) printf("A"); else printf(" ");
if (visualgrid[i][j] & DATAFOUND) printf("d"); else printf(" ");
if (visualgrid[i][j] & DATAGOOD) { printf("D"); total_good++; } else printf(" ");
if (visualgrid[i][j] & DATAPOST) printf("."); else printf(" ");
}
}
printf("\n");
}
printf("\n");
printf("Total Good Sectors: %d\n", total_good);
}
printf("Total Good Sectors: %d\n", total_good);
#endif

return true;
}
Expand Down Expand Up @@ -1407,7 +1411,7 @@ bool a2_nib_format::load(util::random_read &io, uint32_t form_factor, const std:
if (size != expected_size_35t && size != expected_size_40t)
return false;

const auto nr_tracks = size == expected_size_35t? 35 : 40;
const auto nr_tracks = size / nibbles_per_track;

std::vector<uint8_t> nibbles(nibbles_per_track);
for (unsigned track = 0; track < nr_tracks; ++track) {
Expand Down
13 changes: 5 additions & 8 deletions src/lib/formats/ap2_dsk.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,9 @@
***************************************************************************/

#define APPLE2_NIBBLE_SIZE 416
#define APPLE2_SMALL_NIBBLE_SIZE 374
#define APPLE2_STD_TRACK_COUNT 35
#define APPLE2_TRACK_COUNT 40
#define APPLE2_SECTOR_COUNT 16
#define APPLE2_SECTOR_SIZE 256
constexpr int APPLE2_STD_TRACK_COUNT = 35;
constexpr int APPLE2_TRACK_COUNT = 40;
constexpr int APPLE2_SECTOR_SIZE = 256;
Comment on lines -24 to +26
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the scope of these be reduced? Do they need to be in the global namespace? Are the nibble sizes useful for documentation even if they’re currently unused?


/**************************************************************************/
class a2_13sect_format : public floppy_image_format_t
Expand Down Expand Up @@ -145,8 +142,8 @@ class a2_nib_format : public floppy_image_format_t
private:
static constexpr size_t nibbles_per_track = 0x1a00;
static constexpr size_t min_sync_bytes = 4;
static constexpr auto expected_size_35t = 35 * nibbles_per_track;
static constexpr auto expected_size_40t = 40 * nibbles_per_track;
static constexpr auto expected_size_35t = APPLE2_STD_TRACK_COUNT * nibbles_per_track;
static constexpr auto expected_size_40t = APPLE2_TRACK_COUNT * nibbles_per_track;

static std::vector<uint32_t> generate_levels_from_nibbles(const std::vector<uint8_t>& nibbles);
};
Expand Down
Loading