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.

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

new non-working driver: Scopus Sagitta 150/180 serial terminal #2776

Merged
merged 7 commits into from Dec 26, 2017

Conversation

Projects
None yet
3 participants
@felipesanches
Copy link
Contributor

felipesanches commented Nov 5, 2017

No description provided.

@felipesanches

This comment has been minimized.

Copy link
Contributor

felipesanches commented Nov 5, 2017

screenshot at 2017-11-05 03 44 44

@cuavas
Copy link
Member

cuavas left a comment

Sorry for the delay getting to this. Could you please address the few things I've raised? Thanks for continuing to add unusual systems.

required_device<palette_device> m_palette;
required_device<i8275_device> m_crtc;
required_device<i8257_device> m_dma8257;
required_device<cpu_device> m_maincpu;

This comment has been minimized.

@cuavas

cuavas Nov 9, 2017

Member

MAME source is supposed to use tabs for indentation (one tab per scope level), and it's generally supposed to be viewed at 4 spaces per tab (mentioned both here in the source, and here on our wiki). You've got a mixture of spaces and tabs for indentation here, and where you've used spaces, you're assuming tabs are eight spaces. Could you please clean this up?

This comment has been minimized.

@felipesanches

felipesanches Nov 9, 2017

Contributor

sure. Sorry about that.

const uint8_t *m_chargen;

virtual void machine_start() override;
virtual void machine_reset() override;

This comment has been minimized.

@cuavas

cuavas Nov 9, 2017

Member

Please think about protection levels - overridden protected members should be protected, internal things should be private.



// Character generator
const uint8_t *m_chargen;

This comment has been minimized.

@cuavas

cuavas Nov 9, 2017

Member

Please use a required_region_ptr for this so some classes of error can be caught when running -validate rather than causing null pointer errors when running (see e.g. src/mame/drivers/alphatro.cpp for an example).

}

for (i = 0; i < 7; i++) {
bitmap.pix32(y, x + i) = palette[ (pixels & (1U << (7 - i))) != 0 ];

This comment has been minimized.

@cuavas

cuavas Nov 9, 2017

Member

You're using bool in integer context in the expression in parentheses here. Also, you've created a three-entry palette, but this will only ever access two entries. Is there a reason for the third entry?

This comment has been minimized.

@felipesanches

felipesanches Nov 9, 2017

Contributor

This is code copied and adapted from some other driver. It is the first time I deal with the Intel 8275 so I gotta admit this is a bit of poking and guessing and definitely needs to be reviewed and cleaned up.

@felipesanches

This comment has been minimized.

Copy link
Contributor

felipesanches commented Nov 13, 2017

I'll probaby address this next Wednesday (which is a national holiday here in Brazil)

@felipesanches

This comment has been minimized.

Copy link
Contributor

felipesanches commented Nov 19, 2017

@cuavas I've now addressed the issues you had pointed out.

@Robbbert

This comment has been minimized.

Copy link
Contributor

Robbbert commented Dec 26, 2017

Is this ready to go in?

@felipesanches

This comment has been minimized.

Copy link
Contributor

felipesanches commented Dec 26, 2017

yes. It is still non-working because I haven't figured out the logic for handling the keyboard, but I am not actively working on it at the moment, so yes, please merge.

@Robbbert Robbbert merged commit f128fba into mamedev:master Dec 26, 2017

2 checks passed

continuous-integration/tea the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Robbbert

This comment has been minimized.

Copy link
Contributor

Robbbert commented Dec 26, 2017

Please supply the roms (if not already done) to the usual place. Merging.

@felipesanches felipesanches deleted the felipesanches:sagitta180 branch Dec 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment