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.

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

NFC: extract frontend-facing tag state #4893

Merged
merged 1 commit into from Sep 13, 2019
Merged

Conversation

wwylele
Copy link
Member

@wwylele wwylele commented Aug 18, 2019

Because I don't like changing UX for fixing a core bug like in #4890, I decided to PR my proposed alternative. Still, huge thank to @vvanelslande for discovering the bug and provideing research info. I haven't tested my code. Could you help me test it?


Added a new state amiibo_in_range. This state is akin to the real world
physical relationship between a 3DS machine and an amiibo, which is
independent from the service state (or even the machine is powered on or
not). The service state nfc_tag_state is then synchronized with this
physical state on every potential point when the state changes. This
solves the issue where user might load an amiibo before NFC service
initializes, or remove an amiibo after NFC service shutdown, which
previously causes inconsistent state change.

Also removed std::atomic on nfc_tag_state, because

  1. It is already protected by g_hle_lock
  2. It wasn't properly used in the code anyway. For example, there are
    many double loading on this variable, which effectively make it
    non-atomic.

More info on the incoming comment post


This change is Reviewable

Added a new state amiibo_in_range. This state is akin to the real world
physical relationship between a 3DS machine and an amiibo, which is
independent from the service state (or even the machine is powered on or
not). The service state nfc_tag_state is then synchronized with this
physical state on every potential point when the state changes. This
solves the issue where user might load an amiibo before NFC service
initializes, or remove an amiibo after NFC service shutdown, which
previously causes inconsistent state change.

Also removed std::atomic on nfc_tag_state, because
1. It is already protected by g_hle_lock
2. It wasn't properly used in the code anyway. For example, there are
many double loading on this variable, which effectively make it
non-atomic.
@wwylele wwylele requested a review from FearlessTobi Aug 18, 2019
@wwylele
Copy link
Member Author

wwylele commented Aug 18, 2019

NFC state diagram:
https://github.com/wwylele/misc-3ds-diagram/blob/master/nfc.svg

Transitions with black arrow are active transitions requested by game via service code. Transitions with red arrow are passive transitions caused by physical relation change between amiibo and 3DS. The SyncTagState function is called when

  • the physical state changes
  • the service state changes to a state that has a potential passive transition (=Scanning/InRange/OutOfRange)

}

void Module::Interface::RemoveAmiibo() {
std::lock_guard lock(HLE::g_hle_lock);
nfc->nfc_tag_state = Service::NFC::TagState::TagOutOfRange;
nfc->tag_out_of_range_event->Signal();
nfc->amiibo_data = {};
Copy link
Member Author

@wwylele wwylele Aug 18, 2019

Choose a reason for hiding this comment

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

This clearing is removed because we still want to "latch" the amiibo data even when it is removed. This is because the service might be in the TagDataLoaded state when the amiibo is removed, where the game can still read the data afterwards. Hypothetically, the "latching" behaviour is actively done by the service call LoadAmiiboData, which is the only service call that can transit the state to TagDataLoaded. To entirely mimic the architecture, we would have to duplicated amiibo_data, one for data stored in amiibo, and one for data buffered/latched in 3DS memory. Without losing correctness, we simplify this here to one data structure and let it always latch.

@ghost
Copy link

ghost commented Aug 18, 2019

The bug is fixed

@zhaowenlan1779
Copy link
Member

zhaowenlan1779 commented Sep 13, 2019

I'll merge this in 24 hours if no more comments

Copy link
Contributor

@jroweboy jroweboy left a comment

LGTM

@jroweboy jroweboy merged commit 7bfd829 into citra-emu:master Sep 13, 2019
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants